mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-14 19:27:01 -04:00
fix(ui): propagate find-missing errors and timeouts
This commit is contained in:
1 parent
b3d392b499
commit
aa4e3ce822
4 files changed
+241
-6
No files matched your search
@@ -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();
|
||||
|
||||
@@ -1378,6 +1378,25 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option<Re
|
||||
Some(Value::String(_)) => 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<Re
|
||||
if let Some(within) = within {
|
||||
candidates.retain(|candidate| lightcraft_catalog::query::folder_within(&candidate.path, &within));
|
||||
}
|
||||
let work = move || lightcraft_engine::cmd::missing::plan_find_missing(&candidates, &folder);
|
||||
#[cfg(test)]
|
||||
let test_delay = app.ui.find_missing_test_delay;
|
||||
let work = move || {
|
||||
#[cfg(test)]
|
||||
if let Some(d) = test_delay {
|
||||
std::thread::sleep(d);
|
||||
}
|
||||
lightcraft_engine::cmd::missing::plan_find_missing(&candidates, &folder)
|
||||
};
|
||||
let done = |app: &mut LightcraftApp, ctx: &egui::Context, plan: Result<lightcraft_engine::cmd::missing::FindPlan, String>| {
|
||||
// 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<Re
|
||||
unsure = unsure
|
||||
),
|
||||
);
|
||||
app.ui.last_find_missing = Some(v);
|
||||
app.ui.last_find_missing = Some(v.clone());
|
||||
app.ui.last_find_missing_result = Some(Ok(v));
|
||||
}
|
||||
Err(e) => {
|
||||
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}))
|
||||
}
|
||||
|
||||
@@ -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<serde_json::Value>,
|
||||
/// The outcome (success or error) of the last Find Missing Photos task.
|
||||
#[serde(skip)]
|
||||
pub last_find_missing_result: Option<Result<serde_json::Value, String>>,
|
||||
/// Injected delay for Find Missing Photos worker (test-only, for timeout testing).
|
||||
#[cfg(test)]
|
||||
#[serde(skip)]
|
||||
pub find_missing_test_delay: Option<std::time::Duration>,
|
||||
#[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,
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user