From dc28eb5083e14bdcce4207e9519ceda83f94e85a Mon Sep 17 00:00:00 2001 From: Fabio Lissi Date: Mon, 2 Nov 2026 00:25:43 -0600 Subject: [PATCH] Stream batch directory scans and report cancellations accurately --- apps/photocraft-cli/src/batch.rs | 18 +++++++------- crates/engine/src/cmd/file_cmds/batch_run.rs | 12 +++++++--- crates/engine/src/cmd/file_cmds/paths.rs | 16 ++++++------- crates/engine/src/cmd/file_cmds/tests.rs | 10 ++++++++ crates/engine/src/jobs.rs | 9 ++++--- crates/engine/src/jobs/tests.rs | 23 ++++++++++++++++++ crates/ui-egui/src/jobs_ui.rs | 7 ++++++ crates/ui-egui/src/jobs_ui/tests.rs | 25 ++++++++++++++++++++ 8 files changed, 97 insertions(+), 23 deletions(-) diff --git a/apps/photocraft-cli/src/batch.rs b/apps/photocraft-cli/src/batch.rs index 15abd8848c..354c06b58c 100644 --- a/apps/photocraft-cli/src/batch.rs +++ b/apps/photocraft-cli/src/batch.rs @@ -64,15 +64,15 @@ pub(crate) fn batch(a: &BatchArgs, out: &mut dyn Write, err: &mut dyn Write) -> .filter(|input| is_input(Path::new(&input.path))) .collect() } else { - std::fs::read_dir(in_dir) - .map_err(|source| CliError::ReadDir { path: in_dir.to_path_buf(), source })? - .map(|entry| entry.map(|entry| entry.path())) - .collect::>>() - .map_err(|source| CliError::ReadDir { path: in_dir.to_path_buf(), source })? - .into_iter() - .filter(|path| is_input(path)) - .map(|path| photocraft_engine::file_cmds::BatchInput { path: path.to_string_lossy().into_owned(), rel_dir: String::new() }) - .collect() + let mut images = Vec::new(); + let entries = std::fs::read_dir(in_dir).map_err(|source| CliError::ReadDir { path: in_dir.to_path_buf(), source })?; + for entry in entries { + let path = entry.map_err(|source| CliError::ReadDir { path: in_dir.to_path_buf(), source })?.path(); + if is_input(&path) { + images.push(photocraft_engine::file_cmds::BatchInput { path: path.to_string_lossy().into_owned(), rel_dir: String::new() }); + } + } + images }; inputs.sort_by(|a, b| a.path.cmp(&b.path)); let (mut ok, mut failed) = (0, 0); diff --git a/crates/engine/src/cmd/file_cmds/batch_run.rs b/crates/engine/src/cmd/file_cmds/batch_run.rs index 93e8da6d66..66c54b0dea 100644 --- a/crates/engine/src/cmd/file_cmds/batch_run.rs +++ b/crates/engine/src/cmd/file_cmds/batch_run.rs @@ -74,9 +74,15 @@ pub fn list_images_tree(dir: &str, skip: &str, cmd: &str) -> Result>>().map_err(|e| EngineError::Other(format!("{shown}: {e}")))?; - let mut subs: Vec<(String, PathBuf)> = - entries.into_iter().map(|e| e.path()).filter(|p| p.is_dir()).filter_map(|p| Some((p.file_name()?.to_string_lossy().into_owned(), p))).collect(); + let mut subs: Vec<(String, PathBuf)> = Vec::new(); + for entry in rd { + let path = entry.map_err(|e| EngineError::Other(format!("{shown}: {e}")))?.path(); + if path.is_dir() + && let Some(name) = path.file_name() + { + subs.push((name.to_string_lossy().into_owned(), path.clone())); + } + } subs.sort(); // Pushed in reverse so the stack pops them in name order. for (name, sub) in subs.into_iter().rev() { diff --git a/crates/engine/src/cmd/file_cmds/paths.rs b/crates/engine/src/cmd/file_cmds/paths.rs index 63455286ec..68f1c513d6 100644 --- a/crates/engine/src/cmd/file_cmds/paths.rs +++ b/crates/engine/src/cmd/file_cmds/paths.rs @@ -40,13 +40,13 @@ pub(crate) fn write_file(path: &str, _bytes: &[u8]) -> Result<()> { #[cfg(not(target_arch = "wasm32"))] pub fn list_images(dir: &str) -> Result> { let rd = std::fs::read_dir(dir).map_err(|e| EngineError::Other(format!("{dir}: {e}")))?; - let entries = rd.collect::>>().map_err(|e| EngineError::Other(format!("{dir}: {e}")))?; - let mut out: Vec = entries - .into_iter() - .map(|e| e.path()) - .filter(|p| p.is_file() && openable_name(&p.to_string_lossy())) - .map(|p| p.to_string_lossy().into_owned()) - .collect(); + let mut out = Vec::new(); + for entry in rd { + let path = entry.map_err(|e| EngineError::Other(format!("{dir}: {e}")))?.path(); + if path.is_file() && openable_name(&path.to_string_lossy()) { + out.push(path.to_string_lossy().into_owned()); + } + } out.sort(); Ok(out) } @@ -224,7 +224,7 @@ pub fn same_destination(a: impl AsRef, b: impl AsRef a.eq_ignore_ascii_case(b), + (Some(a), Some(b)) => a.to_lowercase() == b.to_lowercase(), _ => a == b, } } diff --git a/crates/engine/src/cmd/file_cmds/tests.rs b/crates/engine/src/cmd/file_cmds/tests.rs index ef9758fa53..d1159aaabc 100644 --- a/crates/engine/src/cmd/file_cmds/tests.rs +++ b/crates/engine/src/cmd/file_cmds/tests.rs @@ -103,6 +103,13 @@ fn linux_distinct_case_destinations_are_not_the_same_file() { assert!(!same_destination(join(&dir, "Image.png"), join(&dir, "image.png"))); } +#[cfg(any(target_os = "macos", target_os = "windows"))] +#[test] +fn unicode_case_variants_cannot_name_a_batch_output_and_its_error_log() { + let dir = tmp("batch-unicode-case-destinations"); + assert!(same_destination(join(&dir, "Ä.png"), join(&dir, "ä.png"))); +} + #[test] fn batch_open_source_edits_each_live_document_and_restores_the_active_one() { let dir = tmp("batch-open-documents"); @@ -179,6 +186,9 @@ fn recursive_batch_reports_an_unreadable_subfolder() { std::fs::set_permissions(&sub, std::fs::Permissions::from_mode(0o700)).unwrap(); if inaccessible { assert!(result.is_err(), "an inaccessible input folder must not be omitted"); + } else { + eprintln!("permission regression skipped: this user can read a mode-000 folder"); + assert!(result.is_ok(), "a privileged reader should still process the folder"); } } diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index 3c2cb9e3b9..b8d5e68ac6 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -701,6 +701,9 @@ impl Session { // Earlier batch outputs are already on disk. Let the worker stop after its current // input and return the completed and skipped lists. if self.jobs.running[i].command == "file.automate.batch" { + if self.jobs.running[i].ctx.cancelled() { + return false; + } self.jobs.running[i].ctx.cancel(); return true; } @@ -1124,9 +1127,9 @@ pub fn specs() -> Vec { enabled: |_| Ok(()), run: |s, p| match p.get("job") { None | Some(Value::Null) => { - let n = s.jobs.running.len(); - s.cancel_all_jobs(); - Ok(json!({"cancelled": n.saturating_sub(s.jobs.running.len())})) + let ids: Vec = s.jobs.running.iter().map(|job| job.id).collect(); + let accepted = ids.into_iter().filter(|id| s.cancel_job(*id)).count(); + Ok(json!({"cancelled": accepted})) } Some(v) => { let id = v.as_u64().ok_or_else(|| EngineError::BadParams { cmd: "jobs.cancel".into(), msg: "`job` must be a job id".into() })?; diff --git a/crates/engine/src/jobs/tests.rs b/crates/engine/src/jobs/tests.rs index 19502cae35..cbb8b74059 100644 --- a/crates/engine/src/jobs/tests.rs +++ b/crates/engine/src/jobs/tests.rs @@ -569,6 +569,29 @@ fn cancelled_batch_job_returns_its_partial_result() { assert_eq!(result["cancelled"], true); } +#[test] +fn cancel_all_counts_an_accepted_batch_cancellation() { + let mut s = Session::new(); + let id = job(s + .start_job( + "file.automate.batch", + json!({}), + "Batch", + false, + |ctx| { + while !ctx.cancelled() { + std::thread::sleep(Duration::from_millis(2)); + } + Ok(json!({"files": [], "cancelled": true})) + }, + |_, result| Ok(result), + ) + .unwrap()); + assert_eq!(s.execute("jobs.cancel", json!({})).unwrap()["cancelled"], 1); + assert_eq!(s.execute("jobs.cancel", json!({})).unwrap()["cancelled"], 0, "a repeated request is not a new cancellation"); + assert!(matches!(wait_event(&mut s, id).outcome, JobOutcome::Done(_))); +} + /// Ask When Opening: a file whose profile doesn't match is decoded but not added; the shell takes /// it once the prompt is answered. Without Ask it opens as usual. #[test] diff --git a/crates/ui-egui/src/jobs_ui.rs b/crates/ui-egui/src/jobs_ui.rs index 00fce3da53..83bd7e2753 100644 --- a/crates/ui-egui/src/jobs_ui.rs +++ b/crates/ui-egui/src/jobs_ui.rs @@ -448,9 +448,12 @@ pub fn cancel(app: &mut PhotocraftApp, id: JobId) -> bool { } fn on_event(app: &mut PhotocraftApp, ctx: &egui::Context, e: JobEvent) { + let cancelled_batch = + e.command == "file.automate.batch" && matches!(&e.outcome, JobOutcome::Done(result) if result.get("cancelled").and_then(Value::as_bool) == Some(true)); // Preferences ▸ General ▸ Beep When Done (off by default): a finished operation beeps, like // Photoshop's cue. Cancelling is the user's own action and stays silent. if matches!(e.outcome, JobOutcome::Done(_)) + && !cancelled_batch && app.session.prefs().general.beep_when_done && let Some(beep) = app.services.beep { @@ -526,6 +529,10 @@ fn on_event(app: &mut PhotocraftApp, ctx: &egui::Context, e: JobEvent) { app.sync_views(); crate::file_ui::print_status(app, result); } + JobOutcome::Done(_) if cancelled_batch => { + app.ui.status = crate::i18n::fmt(tl!("Cancelled {label}"), &[("label", &e.label)]); + app.ui.status_error = false; + } JobOutcome::Done(_) => { app.sync_views(); app.ui.status = e.label; diff --git a/crates/ui-egui/src/jobs_ui/tests.rs b/crates/ui-egui/src/jobs_ui/tests.rs index 4377074ed5..12f6b0901d 100644 --- a/crates/ui-egui/src/jobs_ui/tests.rs +++ b/crates/ui-egui/src/jobs_ui/tests.rs @@ -748,6 +748,31 @@ fn a_finished_job_beeps_when_the_preference_is_on() { assert_eq!(BEEPS.load(Ordering::Relaxed), 1, "one beep for the finished job"); } +#[test] +fn cancelled_batch_with_partial_results_reports_cancellation_without_beeping() { + static BEEPS: AtomicUsize = AtomicUsize::new(0); + fn count_beep() { + BEEPS.fetch_add(1, Ordering::Relaxed); + } + BEEPS.store(0, Ordering::Relaxed); + let mut h = app_harness_with(crate::Services { beep: Some(count_beep), ..Default::default() }); + h.state_mut().run("prefs.set", json!({"path": "general.beepWhenDone", "value": true})).unwrap(); + let ctx = h.ctx.clone(); + crate::jobs_ui::on_event( + h.state_mut(), + &ctx, + photocraft_engine::jobs::JobEvent { + id: JobId(9999), + command: "file.automate.batch".into(), + label: "Batch".into(), + document: None, + outcome: photocraft_engine::jobs::JobOutcome::Done(json!({"files": ["done.png"], "cancelled": true})), + }, + ); + assert!(h.state().ui.status.contains("Cancelled"), "{}", h.state().ui.status); + assert_eq!(BEEPS.load(Ordering::Relaxed), 0); +} + #[test] fn printing_keeps_drawing_and_reports_only_the_final_spooler_result() { for success in [true, false] {