diff --git a/crates/ui-egui/src/menubar.rs b/crates/ui-egui/src/menubar.rs index 04eea2439..70c878731 100644 --- a/crates/ui-egui/src/menubar.rs +++ b/crates/ui-egui/src/menubar.rs @@ -1286,7 +1286,168 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// #1528: `file.findMissing` with `wait: true` must propagate folder validation and search + /// failures as errors instead of returning `Ok(null)`. #[test] + fn find_missing_waited_invalid_folder_returns_error_with_zero_mutations() { + let dir = std::env::temp_dir().join(format!("lc-ui-missing-err-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let dummy = dir.join("dummy.png"); + let img = lightcraft_raster::Rgba8::from_fn(8, 8, |_, _| [10, 20, 30, 255]); + let o = lightcraft_engine::export::ExportOptions { format: lightcraft_engine::export::ExportFormat::Png, ..Default::default() }; + std::fs::write(&dummy, lightcraft_engine::export::encode_image(&img, &o).unwrap()).unwrap(); + + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Default::default()); + app.session.execute("library.import", &json!({"paths": [dummy.to_string_lossy()]})).unwrap(); + // remove the file on disk so the photo is missing + std::fs::remove_file(&dummy).unwrap(); + let id = app.session.catalog.photos().next().unwrap().id; + let original_path = match &app.session.catalog.photo(id).unwrap().source { + lightcraft_catalog::Source::File { path } => path.clone(), + _ => panic!("expected file source"), + }; + let undo_count_before = app.session.undo.len(); + + // 1. Point at a nonexistent directory + let bad_dir = dir.join("nonexistent_subdir"); + let err = run_item(&mut app, "file.findMissing", json!({"folder": bad_dir.to_string_lossy(), "wait": true})).unwrap_err(); + assert!(err.contains("not a folder"), "expected 'not a folder' error, got: {err}"); + assert!(app.ui.toast.as_ref().map(|t| t.0.contains("not a folder")).unwrap_or(false), "toast should reflect failure"); + assert!(app.ui.last_find_missing.is_none(), "last_find_missing must remain None on failure"); + + // 2. Invalid timeout parameters are rejected cleanly + let err = run_item(&mut app, "file.findMissing", json!({"folder": dir.to_string_lossy(), "wait": true, "timeout": -1.0})).unwrap_err(); + assert!(err.contains("non-negative number of seconds"), "{err}"); + let err = run_item(&mut app, "file.findMissing", json!({"folder": dir.to_string_lossy(), "wait": true, "timeout": "invalid"})).unwrap_err(); + assert!(err.contains("number of seconds"), "{err}"); + let err = run_item(&mut app, "file.findMissing", json!({"folder": dir.to_string_lossy(), "wait": true, "timeout": 1e300})).unwrap_err(); + assert!(err.contains("too large"), "{err}"); + + // Catalog and photo path must not have been mutated + assert_eq!(app.session.undo.len(), undo_count_before, "no undo steps added on failed search"); + let path_after = match &app.session.catalog.photo(id).unwrap().source { + lightcraft_catalog::Source::File { path } => path.clone(), + _ => panic!("expected file source"), + }; + assert_eq!(path_after, original_path, "missing photo path must not change on failure"); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// #1528: `file.findMissing` with `wait: true` returns structured results on success, + /// and background mode (`wait: false` or omitted) continues returning `{"background": true}`. + #[test] + fn find_missing_waited_structured_results_and_background_mode() { + let dir = std::env::temp_dir().join(format!("lc-ui-missing-bg-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("orig")).unwrap(); + std::fs::create_dir_all(dir.join("found")).unwrap(); + let img = lightcraft_raster::Rgba8::from_fn(8, 8, |_, _| [10, 20, 30, 255]); + let o = lightcraft_engine::export::ExportOptions { format: lightcraft_engine::export::ExportFormat::Png, ..Default::default() }; + std::fs::write(dir.join("orig/pic.png"), lightcraft_engine::export::encode_image(&img, &o).unwrap()).unwrap(); + + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Default::default()); + app.session.execute("library.import", &json!({"paths": [dir.join("orig").to_string_lossy()]})).unwrap(); + std::fs::rename(dir.join("orig/pic.png"), dir.join("found/pic.png")).unwrap(); + + // Background mode (wait omitted): returns {"background": true} immediately + let bg = run_item(&mut app, "file.findMissing", json!({"folder": dir.join("found").to_string_lossy()})).unwrap(); + assert_eq!(bg, json!({"background": true})); + + // Wait for background task to complete + let ctx = app.tasks.repaint.clone().unwrap_or_default(); + assert!(crate::tasks::wait(&mut app, &ctx, std::time::Duration::from_secs(5))); + let last = app.ui.last_find_missing.as_ref().unwrap(); + assert_eq!(last["found"].as_array().map(Vec::len), Some(1)); + + // Now move again and run with wait: true -> returns structured results directly + std::fs::create_dir_all(dir.join("found2")).unwrap(); + std::fs::rename(dir.join("found/pic.png"), dir.join("found2/pic.png")).unwrap(); + let r = run_item(&mut app, "file.findMissing", json!({"folder": dir.join("found2").to_string_lossy(), "wait": true})).unwrap(); + assert!(r.get("found").is_some(), "must have 'found' array: {r}"); + assert!(r.get("missing").is_some(), "must have 'missing' count: {r}"); + assert!(r.get("ambiguous").is_some(), "must have 'ambiguous' array: {r}"); + assert_eq!(r["found"].as_array().map(Vec::len), Some(1)); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// #1528: `file.findMissing` with `wait: true` when timed out returns an actionable error + /// indicating timeout and that the search continues in the background, rather than returning `Ok(null)`. + #[test] + fn find_missing_waited_timeout_behavior() { + let dir = std::env::temp_dir().join(format!("lc-ui-missing-to-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Default::default()); + // Injected test-only worker delay so wait times out deterministically + app.ui.find_missing_test_delay = Some(std::time::Duration::from_millis(100)); + let err = run_item(&mut app, "file.findMissing", json!({"folder": dir.to_string_lossy(), "wait": true, "timeout": 0.005})).unwrap_err(); + + assert!(err.contains("search timed out"), "error should mention timeout: {err}"); + assert!(err.contains("search continues in the background"), "error should document continuation: {err}"); + + // The background task continues running in app.tasks + assert!(app.tasks.is_running("Find Missing Photos"), "task should continue running in background"); + + // Clean up the running task so no worker hangs + let ctx = app.tasks.repaint.clone().unwrap_or_default(); + assert!(crate::tasks::wait_for(&mut app, &ctx, "Find Missing Photos", std::time::Duration::from_secs(5))); + assert!(!app.tasks.is_running("Find Missing Photos")); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// #1528: `file.findMissing` with `wait: true` must return when its own search finishes, + /// even if unrelated background tasks remain active in `app.tasks`. + #[test] + fn find_missing_waited_completes_while_unrelated_task_remains_active() { + let dir = std::env::temp_dir().join(format!("lc-ui-missing-multi-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("orig")).unwrap(); + std::fs::create_dir_all(dir.join("found")).unwrap(); + let img = lightcraft_raster::Rgba8::from_fn(8, 8, |_, _| [10, 20, 30, 255]); + let o = lightcraft_engine::export::ExportOptions { format: lightcraft_engine::export::ExportFormat::Png, ..Default::default() }; + std::fs::write(dir.join("orig/pic.png"), lightcraft_engine::export::encode_image(&img, &o).unwrap()).unwrap(); + + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Default::default()); + app.session.execute("library.import", &json!({"paths": [dir.join("orig").to_string_lossy()]})).unwrap(); + std::fs::rename(dir.join("orig/pic.png"), dir.join("found/pic.png")).unwrap(); + + // Spawn an unrelated background task that blocks on a channel until released + let (tx_unblock, rx_unblock) = std::sync::mpsc::channel::<()>(); + crate::tasks::spawn( + &mut app, + "Unrelated Long Background Task", + None, + move || { + let _ = rx_unblock.recv(); + }, + |_app, _ctx, _| {}, + ) + .unwrap(); + assert!(app.tasks.is_running("Unrelated Long Background Task")); + + // Run file.findMissing with wait: true: must complete without waiting for the unrelated task + let r = run_item(&mut app, "file.findMissing", json!({"folder": dir.join("found").to_string_lossy(), "wait": true, "timeout": 5.0})).unwrap(); + assert_eq!(r["found"].as_array().map(Vec::len), Some(1), "Find Missing must complete successfully: {r}"); + + // The unrelated task must STILL be active + assert!(app.tasks.is_running("Unrelated Long Background Task"), "unrelated task must still be running"); + + // Clean up the blocked background task + let _ = tx_unblock.send(()); + let ctx = app.tasks.repaint.clone().unwrap_or_default(); + assert!(crate::tasks::wait(&mut app, &ctx, std::time::Duration::from_secs(5))); + assert!(app.tasks.is_empty()); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn auto_tag_from_tracklog() { let dir = std::env::temp_dir().join(format!("lc-ui-tracklog-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 141d6d782..4dec68ad3 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1378,6 +1378,25 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option return Some(Err("file.findMissing: `within` must not be empty".into())), Some(value) => return Some(Err(format!("file.findMissing: `within` must be a folder path, not {value}"))), }; + let wait = p.get("wait").and_then(Value::as_bool).unwrap_or(false); + let timeout = match p.get("timeout") { + None | Some(Value::Null) => std::time::Duration::from_secs(600), + Some(Value::Number(n)) => { + if let Some(s) = n.as_f64() { + if s.is_finite() && s >= 0.0 { + match std::time::Duration::try_from_secs_f64(s) { + Ok(d) => d, + Err(_) => return Some(Err("file.findMissing: `timeout` is too large".into())), + } + } else { + return Some(Err("file.findMissing: `timeout` must be a non-negative number of seconds".into())); + } + } else { + return Some(Err("file.findMissing: `timeout` must be a number of seconds".into())); + } + } + Some(other) => return Some(Err(format!("file.findMissing: `timeout` must be a number of seconds, not {other}"))), + }; let title = if within.is_some() { "Locate Missing Files…" } else { "Find Missing Photos…" }; let folder = match p.get("folder").and_then(Value::as_str) { Some(f) => Some(f.to_string()), @@ -1399,7 +1418,15 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option| { // relinked under the session as it is now: photos relinked meanwhile and files // now in use are skipped @@ -1424,19 +1451,31 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option { + app.toast(ctx, e.clone()); + app.ui.last_find_missing_result = Some(Err(e)); } - Err(e) => app.toast(ctx, e), } }; app.ui.last_find_missing = None; + app.ui.last_find_missing_result = None; if let Err(e) = crate::tasks::spawn(app, LABEL, Some("findMissing"), work, done) { return Some(Err(e)); } - if p.get("wait").and_then(Value::as_bool).unwrap_or(false) { + if wait { let ctx = app.tasks.repaint.clone().unwrap_or_default(); - crate::tasks::wait(app, &ctx, std::time::Duration::from_secs(600)); - return Some(Ok(app.ui.last_find_missing.clone().unwrap_or(Value::Null))); + let completed = crate::tasks::wait_for(app, &ctx, LABEL, timeout); + if !completed { + return Some(Err("Find Missing Photos: search timed out (search continues in the background)".into())); + } + return match app.ui.last_find_missing_result.take() { + Some(Ok(v)) => Some(Ok(v)), + Some(Err(e)) => Some(Err(e)), + None => Some(Err("Find Missing Photos: worker terminated unexpectedly".into())), + }; } Ok(json!({"background": true})) } diff --git a/crates/ui-egui/src/state.rs b/crates/ui-egui/src/state.rs index 6c4dd3f6a..8e5021b5a 100644 --- a/crates/ui-egui/src/state.rs +++ b/crates/ui-egui/src/state.rs @@ -736,6 +736,13 @@ pub struct UiState { /// The result of the last Find Missing Photos (it searches in the background). #[serde(skip)] pub last_find_missing: Option, + /// The outcome (success or error) of the last Find Missing Photos task. + #[serde(skip)] + pub last_find_missing_result: Option>, + /// Injected delay for Find Missing Photos worker (test-only, for timeout testing). + #[cfg(test)] + #[serde(skip)] + pub find_missing_test_delay: Option, #[serde(skip)] pub status: String, #[serde(skip)] @@ -1140,6 +1147,9 @@ impl Default for UiState { reference: None, toast: None, last_find_missing: None, + last_find_missing_result: None, + #[cfg(test)] + find_missing_test_delay: None, status: String::new(), dialog: None, } diff --git a/crates/ui-egui/src/tasks.rs b/crates/ui-egui/src/tasks.rs index fb5a1f031..70abd629a 100644 --- a/crates/ui-egui/src/tasks.rs +++ b/crates/ui-egui/src/tasks.rs @@ -116,3 +116,28 @@ pub fn wait(app: &mut LightcraftApp, ctx: &egui::Context, timeout: std::time::Du app.tasks.is_empty() } } + +/// Wait for a specific task by label and apply finished tasks. Returns `true` once the labeled +/// task finishes (or is removed), or `false` on timeout. Other background tasks may continue running. +pub fn wait_for(app: &mut LightcraftApp, ctx: &egui::Context, label: &str, timeout: std::time::Duration) -> bool { + #[cfg(not(target_arch = "wasm32"))] + { + let t0 = std::time::Instant::now(); + loop { + poll(app, ctx); + if !app.tasks.is_running(label) { + return true; + } + if t0.elapsed() > timeout { + return false; + } + std::thread::sleep(std::time::Duration::from_millis(2)); + } + } + #[cfg(target_arch = "wasm32")] + { + let _ = timeout; + poll(app, ctx); + !app.tasks.is_running(label) + } +}