diff --git a/crates/engine/src/activity.rs b/crates/engine/src/activity.rs index 654b6db44..6fc618039 100644 --- a/crates/engine/src/activity.rs +++ b/crates/engine/src/activity.rs @@ -73,7 +73,7 @@ impl Unit { pub struct TaskInfo { pub id: u64, /// A stable id: `import`, `export`, `scan`, `previews`, `smartPreviews`, `lightroom`, `merge`, `download`, - /// `faces`, `denoise`, `findMissing`. + /// `faces`, `denoise`, `findMissing`, `applePhotos`. pub kind: &'static str, /// English; the frontend translates it. pub label: String, @@ -100,6 +100,12 @@ struct Entry { detail: Mutex, cancel: Option>, cancellable: AtomicBool, + /// Held while a cancel is decided ([`Activity::cancel`], [`Activity::cancel_all`]) and while + /// cancelling is switched ([`TaskGuard::set_cancellable`], [`TaskHandle::set_cancellable`]): a + /// step that turns it off and then reads the cancel flag either sees a cancel that was + /// accepted, or makes the cancel fail. Without it a cancel could pass the check, the step + /// start, and the flag be set after the step's last look at it: accepted but ignored. + gate: Mutex<()>, } fn lock(m: &Mutex) -> MutexGuard<'_, T> { @@ -111,6 +117,23 @@ fn capped(text: &str) -> String { } impl Entry { + fn set_cancellable(&self, yes: bool) { + let _gate = lock(&self.gate); + self.cancellable.store(yes, Ordering::Relaxed); + } + + /// Set the cancel flag if the task can be cancelled now (see [`Entry::gate`]). + fn try_cancel(&self) -> bool { + let _gate = lock(&self.gate); + match &self.cancel { + Some(flag) if self.is_cancellable() => { + flag.store(true, Ordering::Relaxed); + true + } + _ => false, + } + } + fn is_cancellable(&self) -> bool { self.cancel.is_some() && self.cancellable.load(Ordering::Relaxed) } @@ -165,6 +188,7 @@ impl Activity { detail: Mutex::new(String::new()), cancellable: AtomicBool::new(cancel.is_some()), cancel, + gate: Mutex::new(()), }); lock(&self.inner.tasks).push(entry.clone()); TaskGuard { activity: self.clone(), entry } @@ -180,26 +204,13 @@ impl Activity { pub fn cancel(&self, id: u64) -> Result<(), String> { let tasks = lock(&self.inner.tasks); let entry = tasks.iter().find(|e| e.id == id).ok_or_else(|| format!("no task {id}"))?; - match (&entry.cancel, entry.is_cancellable()) { - (Some(flag), true) => { - flag.store(true, Ordering::Relaxed); - Ok(()) - } - _ => Err(format!("{} can't be cancelled", entry.label)), - } + if entry.try_cancel() { Ok(()) } else { Err(format!("{} can't be cancelled", entry.label)) } } /// Ask every cancellable task to stop; returns how many were asked. pub fn cancel_all(&self) -> usize { let tasks = lock(&self.inner.tasks); - let mut n = 0; - for e in tasks.iter().filter(|e| e.is_cancellable()) { - if let Some(flag) = &e.cancel { - flag.store(true, Ordering::Relaxed); - n += 1; - } - } - n + tasks.iter().filter(|e| e.try_cancel()).count() } /// Tasks that quitting would cut short: cancellable ones not already stopping. @@ -235,7 +246,7 @@ impl TaskGuard { /// A step that must not be cut short (the Lightroom import's commit) turns cancelling off for its duration. pub fn set_cancellable(&self, yes: bool) { - self.entry.cancellable.store(yes, Ordering::Relaxed); + self.entry.set_cancellable(yes); } pub fn cancel_flag(&self) -> Option> { @@ -283,6 +294,13 @@ impl TaskHandle { pub fn is_cancelled(&self) -> bool { self.entry.is_cancelled() } + + /// [`TaskGuard::set_cancellable`] from the worker, at the moment its step starts (an export + /// handing its files to Apple Photos, which can't be stopped once asked). Read the cancel flag + /// after turning it off: a cancel accepted before is seen then, any later one is refused. + pub fn set_cancellable(&self, yes: bool) { + self.entry.set_cancellable(yes); + } } #[cfg(test)] @@ -380,6 +398,35 @@ mod tests { assert_eq!(a.list().len(), 1); } + /// Codex review of #643: a worker that turns cancelling off and then reads the cancel flag + /// (an export handing its files to Apple Photos) must either see a cancel that was accepted or + /// make it fail, never let one be accepted and then ignored. Here the test holds the task's gate + /// as a cancel does while it decides: the worker's switch waits for it, and then sees the cancel. + #[test] + fn a_step_that_turns_cancelling_off_sees_or_refuses_every_cancel() { + let a = Activity::default(); + let flag = Arc::new(AtomicBool::new(false)); + let g = a.start("export", "Exporting", Cancel::Flag(flag.clone())); + let handle = g.handle(); + let gate = lock(&g.entry.gate); // a cancel deciding + let (tx, rx) = std::sync::mpsc::channel(); + let (h, f) = (handle.clone(), flag.clone()); + let worker = std::thread::spawn(move || { + h.set_cancellable(false); + tx.send(f.load(Ordering::Relaxed)).unwrap(); + }); + assert!(rx.recv_timeout(std::time::Duration::from_millis(100)).is_err(), "the switch waits for the cancel"); + flag.store(true, Ordering::Relaxed); // the cancel goes through... + drop(gate); + assert!(rx.recv_timeout(std::time::Duration::from_secs(10)).unwrap(), "...and the step sees it"); + worker.join().unwrap(); + // once it is off, cancels are refused + let g2 = a.start("export", "Exporting", Cancel::Yes); + g2.handle().set_cancellable(false); + assert!(a.cancel(g2.id()).is_err()); + assert_eq!(a.cancel_all(), 0, "{:?}", a.list()); + } + #[test] fn handle_updates_from_another_thread() { let a = Activity::default(); diff --git a/crates/engine/src/apple_photos.rs b/crates/engine/src/apple_photos.rs index f59468b55..fd1c6f24b 100644 --- a/crates/engine/src/apple_photos.rs +++ b/crates/engine/src/apple_photos.rs @@ -28,6 +28,8 @@ use std::time::Duration; use serde_json::{Value, json}; +use crate::activity::{Activity, Cancel, TaskGuard}; + /// Run by osascript with `argv` = [`MARKER`], the album name (empty: none), then the files' POSIX /// paths. Prints the imported media items' ids, one per line. /// @@ -469,7 +471,7 @@ impl Imports { return Err("an album name can't contain a NUL character".into()); } let job = self.begin(0, album, true)?; - Ok(Reservation { runner, album: album.to_string(), job: Some(job) }) + Ok(Reservation { runner, album: album.to_string(), job: Some(job), activity: None }) } /// [`Imports::wait_idle_within`] with the [`WaitLimits::default`]. @@ -542,17 +544,39 @@ impl crate::Session { } } -/// Ask Photos to import `paths` (checked; `album` trimmed) for `job`: here, or on a worker thread. -fn launch(runner: &Runner, job: &Arc, paths: Vec, album: String, background: bool) { +/// The activity stack's kind for an Apple Photos import of its own (`activity.list`). +pub const ACTIVITY_KIND: &str = "applePhotos"; +/// ...and its label (translated by the frontend). +pub const ACTIVITY_LABEL: &str = "Adding to Apple Photos"; + +/// A row in the activity stack for an import that runs on its own (not as the last step of an +/// export, whose row covers it): no ✕ (Photos can't be stopped once asked), no count (Photos +/// reports none until it is done), the album as its detail. +fn row(activity: Option<&Activity>, album: &str) -> Option { + let row = activity?.start(ACTIVITY_KIND, ACTIVITY_LABEL, Cancel::No); + row.progress(0, 0); + if !album.is_empty() { + row.detail(album); + } + Some(row) +} + +/// Ask Photos to import `paths` (checked; `album` trimmed) for `job`: here, or on a worker thread, +/// holding `row` (the activity stack's) until Photos is done. +fn launch(runner: &Runner, job: &Arc, paths: Vec, album: String, background: bool, row: Option) { job.requested.store(paths.len(), Ordering::Relaxed); *job.started.lock().unwrap_or_else(PoisonError::into_inner) = Some(std::time::Instant::now()); job.exporting.store(false, Ordering::Relaxed); if !background { + let _row = row; run_to_completion(runner, job, &paths, &album); return; } let (r, j) = (runner.clone(), job.clone()); - let started = std::thread::Builder::new().name("apple-photos".into()).spawn(move || run_to_completion(&r, &j, &paths, &album)); + let started = std::thread::Builder::new().name("apple-photos".into()).spawn(move || { + let _row = row; + run_to_completion(&r, &j, &paths, &album) + }); if let Err(e) = started { job.finish(Outcome::Failed(format!("Couldn't start adding the photos to Apple Photos: {e}"))); } @@ -581,13 +605,22 @@ fn run_to_completion(runner: &Runner, job: &ImportJob, paths: &[String], album: } /// Add `paths` to Photos as a job of `imports`: on a worker thread (`background`; the job is -/// returned running, and announced by the app when it ends) or here (returned finished). An -/// error, and no job, for a bad request or while another import is reserved or running. -pub fn import_job(runner: &Runner, imports: &Imports, paths: Vec, album: &str, background: bool) -> Result, String> { +/// returned running, and announced by the app when it ends) or here (returned finished), with a +/// row in `activity` while Photos works. An error, and no job, for a bad request or while +/// another import is reserved or running. +pub fn import_job( + runner: &Runner, + imports: &Imports, + paths: Vec, + album: &str, + background: bool, + activity: Option<&Activity>, +) -> Result, String> { let album = check(&paths, album)?; let job = imports.begin(paths.len(), &album, false)?; job.quiet.store(!background, Ordering::Relaxed); - launch(runner, &job, paths, album, background); + let row = row(activity, &album); + launch(runner, &job, paths, album, background, row); Ok(job) } @@ -598,9 +631,18 @@ pub struct Reservation { runner: Runner, album: String, job: Option>, + /// Where [`Reservation::start`]'s import shows while Photos works. + activity: Option, } impl Reservation { + /// Show the import [`Reservation::start`] leaves running in `activity`. ([`Reservation::run`] + /// runs on the export's worker, and the export's own row covers it.) + pub fn shown_in(mut self, activity: Activity) -> Self { + self.activity = Some(activity); + self + } + pub fn job(&self) -> Option<&Arc> { self.job.as_ref() } @@ -633,7 +675,7 @@ impl Reservation { pub fn run(mut self, files: &[Value], cancelled: bool) -> Value { match self.take(files, cancelled) { Ok((job, paths)) => { - launch(&self.runner, &job, paths, self.album.clone(), false); + launch(&self.runner, &job, paths, self.album.clone(), false, None); job.json() } Err(done) => done, @@ -646,7 +688,8 @@ impl Reservation { match self.take(files, false) { Ok((job, paths)) => { job.quiet.store(false, Ordering::Relaxed); - launch(&self.runner, &job, paths, self.album.clone(), true); + let row = row(self.activity.as_ref(), &self.album); + launch(&self.runner, &job, paths, self.album.clone(), true, row); job.json() } Err(done) => done, @@ -984,7 +1027,7 @@ mod tests { let reserved = imports.reserve(runner.clone(), "Trip").unwrap(); assert_eq!(reserved.job().map(|j| j.json()), Some(json!({"job": 1, "running": true, "exporting": true, "requested": 0, "album": "Trip"}))); // while the export writes: other imports and exports are refused, naming it - let e = import_job(&runner, &imports, paths.clone(), "", false).unwrap_err(); + let e = import_job(&runner, &imports, paths.clone(), "", false, None).unwrap_err(); assert!(e.contains("An export is about to add its files to Apple Photos (import 1)"), "{e}"); assert!(imports.reserve(runner.clone(), "").err().unwrap().contains("(import 1)")); assert!(calls.lock().unwrap().is_empty()); @@ -1003,7 +1046,7 @@ mod tests { assert!(out["error"].as_str().unwrap().contains("no such file"), "{out}"); assert_eq!(calls.lock().unwrap().len(), 1); assert!(imports.running().is_none()); - assert_eq!(import_job(&runner, &imports, paths, "", false).unwrap().id, 4); + assert_eq!(import_job(&runner, &imports, paths, "", false, None).unwrap().id, 4); let _ = std::fs::remove_dir_all(dir); } @@ -1017,11 +1060,11 @@ mod tests { let imports = Imports::default(); let stopped = |j: &ImportJob| j.json()["error"].as_str().is_some_and(|e| e.contains("stopped unexpectedly")); // waiting here - let job = import_job(&panicky, &imports, paths.clone(), "", false).unwrap(); + let job = import_job(&panicky, &imports, paths.clone(), "", false, None).unwrap(); assert!(job.finished() && stopped(&job), "{}", job.json()); assert!(imports.running().is_none()); // on a worker - let job = import_job(&panicky, &imports, paths.clone(), "", true).unwrap(); + let job = import_job(&panicky, &imports, paths.clone(), "", true, None).unwrap(); let t0 = std::time::Instant::now(); while !job.finished() { assert!(t0.elapsed() < Duration::from_secs(20)); @@ -1040,7 +1083,7 @@ mod tests { } assert!(stopped(&job)); // the slot is free: the next import runs - assert_eq!(import_job(&good, &imports, paths, "", false).unwrap().json()["imported"], 1); + assert_eq!(import_job(&good, &imports, paths, "", false, None).unwrap().json()["imported"], 1); let _ = std::fs::remove_dir_all(dir); } @@ -1050,7 +1093,7 @@ mod tests { let (dir, paths) = files("idle", 1); let (runner, go) = delayed(); let imports = Imports::default(); - let job = import_job(&runner, &imports, paths, "", true).unwrap(); + let job = import_job(&runner, &imports, paths, "", true, None).unwrap(); assert!(!job.finished()); let release = std::thread::spawn(move || { std::thread::sleep(Duration::from_millis(200)); @@ -1099,7 +1142,7 @@ mod tests { assert!(t0.elapsed() >= Duration::from_millis(1000), "{:?}", t0.elapsed()); assert_eq!(exporter.join().unwrap()["imported"], 2); // an import past its own limit is given up on at its deadline (not the cap) - let job = import_job(&taking(3000), &imports, paths.clone(), "", true).unwrap(); + let job = import_job(&taking(3000), &imports, paths.clone(), "", true, None).unwrap(); let t0 = std::time::Instant::now(); assert!(!imports.wait_idle_within(limits)); let waited = t0.elapsed(); @@ -1116,7 +1159,7 @@ mod tests { drop(held); assert!(imports.wait_idle_within(limits)); // and the cap holds over everything - let job = import_job(&taking(1000), &imports, paths, "", true).unwrap(); + let job = import_job(&taking(1000), &imports, paths, "", true, None).unwrap(); let t0 = std::time::Instant::now(); assert!(!imports.wait_idle_within(WaitLimits { cap: Duration::from_millis(200), ..limits })); assert!(t0.elapsed() < Duration::from_millis(900)); @@ -1147,11 +1190,11 @@ mod tests { let (runner, go) = delayed(); let imports = Imports::default(); let t0 = std::time::Instant::now(); - let job = import_job(&runner, &imports, paths.clone(), " Trip ", true).unwrap(); + let job = import_job(&runner, &imports, paths.clone(), " Trip ", true, None).unwrap(); assert!(t0.elapsed() < Duration::from_secs(5), "the caller doesn't wait for Photos"); assert_eq!(job.json(), json!({"job": 1, "running": true, "requested": 2, "album": "Trip"})); assert_eq!(imports.running().map(|j| j.id), Some(1)); - let e = import_job(&runner, &imports, paths.clone(), "", true).unwrap_err(); + let e = import_job(&runner, &imports, paths.clone(), "", true, None).unwrap_err(); assert!(e.contains("still adding 2 file(s) from an earlier request (import 1)"), "{e}"); let e = imports.reserve(runner.clone(), "").err().unwrap(); assert!(e.contains("still adding"), "{e}"); @@ -1165,7 +1208,7 @@ mod tests { assert_eq!((done["running"].as_bool(), done["imported"].as_u64(), done["album"].as_str()), (Some(false), Some(2), Some("Trip")), "{done}"); // finished: the next one may start; a failed one says why let (fails, _) = fake(Err(RunError::Spawn("denied".into()))); - let job = import_job(&fails, &imports, paths, "", false).unwrap(); + let job = import_job(&fails, &imports, paths, "", false, None).unwrap(); assert_eq!(job.id, 2); assert!(job.json()["error"].as_str().unwrap().contains("denied")); assert!(imports.running().is_none() && imports.latest().is_some_and(|j| j.id == 2)); @@ -1178,7 +1221,7 @@ mod tests { let (runner, _) = fake(ok("A\n")); let imports = Imports::default(); for _ in 0..KEPT_JOBS + 3 { - import_job(&runner, &imports, paths.clone(), "", false).unwrap(); + import_job(&runner, &imports, paths.clone(), "", false, None).unwrap(); } let all = imports.all(); assert_eq!(all.len(), KEPT_JOBS); @@ -1187,6 +1230,61 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + /// The activity stack (issue #345): an import of its own shows a row while Photos works (no ✕, + /// no count, the album as detail) and loses it when Photos is done; an export's own import + /// shows none of its own, the export's row covers it. + #[test] + fn imports_of_their_own_show_in_the_activity_stack() { + let (dir, paths) = files("activity", 2); + let activity = Activity::default(); + let seen = Arc::new(Mutex::new(Vec::new())); + // Photos answers when the test lets it (`go`), so no assertion races the import's end + let (go, wait) = std::sync::mpsc::channel::<()>(); + let wait = Mutex::new(wait); + let (a, rows) = (activity.clone(), seen.clone()); + let runner: Runner = Arc::new(move |args: &[String], _| { + rows.lock().unwrap().push(a.list()); + let _ = wait.lock().unwrap().recv_timeout(Duration::from_secs(20)); + let n = args.len() - args.iter().position(|x| x == MARKER).unwrap() - 2; + Ok(Output { status: Some(0), stdout: "ID\n".repeat(n), stderr: String::new() }) + }); + let imports = Imports::default(); + let ended = |job: &ImportJob| { + let deadline = std::time::Instant::now() + Duration::from_secs(20); + while !(job.finished() && activity.list().is_empty()) { + assert!(std::time::Instant::now() < deadline, "the import ends and its row goes"); + std::thread::sleep(Duration::from_millis(5)); + } + }; + // waiting for Photos here + go.send(()).unwrap(); + let job = import_job(&runner, &imports, paths.clone(), "Trip", false, Some(&activity)).unwrap(); + assert_eq!(job.json()["imported"], 2); + assert!(activity.list().is_empty(), "the row goes when Photos is done"); + let during = seen.lock().unwrap().last().cloned().unwrap(); + assert_eq!(during.len(), 1, "{during:?}"); + assert_eq!((during[0].kind, during[0].label.as_str(), during[0].detail.as_str()), (ACTIVITY_KIND, ACTIVITY_LABEL, "Trip")); + assert_eq!((during[0].cancellable, during[0].total), (false, 0), "no ✕ and no count"); + // on a worker thread: the row stays while Photos works, and goes once the import ends + let job = import_job(&runner, &imports, paths.clone(), "", true, Some(&activity)).unwrap(); + assert_eq!(activity.list().first().map(|r| r.kind), Some(ACTIVITY_KIND)); + go.send(()).unwrap(); + ended(&job); + // an export's own import: the export's row covers it + let exported: Vec = paths.iter().map(|p| json!({"path": p})).collect(); + go.send(()).unwrap(); + imports.reserve(runner.clone(), "").unwrap().shown_in(activity.clone()).run(&exported, false); + assert!(seen.lock().unwrap().last().unwrap().is_empty(), "no row of its own while the export's worker waits"); + // ...unless it is left running on its own once the export is done + let started = imports.reserve(runner.clone(), "").unwrap().shown_in(activity.clone()).start(&exported); + assert_eq!(started["running"], true); + assert_eq!(activity.list().first().map(|r| r.kind), Some(ACTIVITY_KIND)); + go.send(()).unwrap(); + let job = imports.get(started["job"].as_u64().unwrap()).unwrap(); + ended(&job); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn timeouts_grow_with_the_batch_and_stay_bounded() { assert_eq!(timeout_for(1), Duration::from_secs(605)); diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index dafcb9a33..e93d04175 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -115,10 +115,17 @@ fn add_to_photos(s: &mut Session, p: &Value) -> Result { // in the middle of the import let wait = match p.get("wait") { None | Some(Value::Null) => !s.apple_photos_background, + // the desktop app answers on its window's thread, which never waits for Photos + Some(Value::Bool(true)) if s.apple_photos_background => { + return Err(bad( + ID, + "`wait: true` isn't available in the desktop app: the import runs in the background; its outcome comes from export.photosImports {job}", + )); + } Some(Value::Bool(b)) => *b, Some(_) => return Err(bad(ID, "`wait` must be true or false")), }; - let job = crate::apple_photos::import_job(&runner, &s.apple_photos_imports, paths, album, !wait).map_err(|e| bad(ID, e))?; + let job = crate::apple_photos::import_job(&runner, &s.apple_photos_imports, paths, album, !wait, Some(&s.activity)).map_err(|e| bad(ID, e))?; match job.outcome() { Some(crate::apple_photos::Outcome::Failed(e)) => Err(bad(ID, e)), _ => Ok(job.json()), @@ -193,7 +200,7 @@ pub fn specs() -> Vec { "Add to Apple Photos", [], None, - "{paths: [absolute file paths], album?: name (an album at the top level of Photos, made when missing; default none), wait?} → once Photos is done, {job, running: false, requested, album, imported, ids: [Photos media item ids], warning?}; with wait: false, at once {job, running: true, requested, album} and the outcome later from export.photosImports {job}. wait defaults to true without a window (lightcraft-cli, the MCP server) and to false in the desktop app (its control requests are answered on the UI thread). macOS: asks Photos to import the files (the first time, macOS asks the user to allow LightCraft to control Photos). One import runs at a time, and an export adding to Photos reserves it before writing: another is refused until it ends. app.export does this for the files it writes with `addToPhotos: true`", + "{paths: [absolute file paths], album?: name (an album at the top level of Photos, made when missing; default none), wait?} → once Photos is done, {job, running: false, requested, album, imported, ids: [Photos media item ids], warning?}; with wait: false, at once {job, running: true, requested, album} and the outcome later from export.photosImports {job}. wait defaults to true without a window (lightcraft-cli, the MCP server) and to false in the desktop app, where wait: true is refused (its control requests are answered on the UI thread, which never waits for Photos). macOS: asks Photos to import the files (the first time, macOS asks the user to allow LightCraft to control Photos). One import runs at a time, and an export adding to Photos reserves it before writing: another is refused until it ends. app.export does this for the files it writes with `addToPhotos: true`", photos_enabled, add_to_photos )); diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index 8bbfbead5..b7ffaab03 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -1665,7 +1665,9 @@ impl crate::Session { #[cfg(not(target_arch = "wasm32"))] { let runner = self.apple_photos.clone().ok_or_else(|| crate::apple_photos::unavailable().to_string())?; - AfterExport::apple_photos(runner, &o.photos_album, &self.apple_photos_imports) + let mut after = AfterExport::apple_photos(runner, &o.photos_album, &self.apple_photos_imports)?; + after.apple_photos = after.apple_photos.map(|r| r.shown_in(self.activity.clone())); + Ok(after) } #[cfg(target_arch = "wasm32")] Err("Apple Photos is only available on macOS".into()) diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index 7969b799d..392cb0e53 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -647,9 +647,12 @@ mod apple_photos { let all = s.execute("export.photosImports", &json!({})).unwrap(); assert_eq!(all["imports"].as_array().map(Vec::len), Some(1)); assert!(s.execute("export.photosImports", &json!({"job": 7})).unwrap_err().to_string().contains("no import 7")); + // the window's thread never waits for Photos: `wait: true` is refused here, before any import + let e = s.execute("export.addToPhotos", &json!({"paths": paths, "wait": true})).unwrap_err().to_string(); + assert!(e.contains("isn't available in the desktop app"), "{e}"); // finished: the next one may start go.send(()).unwrap(); - assert_eq!(s.execute("export.addToPhotos", &json!({"paths": paths, "wait": true})).unwrap()["job"], 2); + assert_eq!(s.execute("export.addToPhotos", &json!({"paths": paths})).unwrap()["job"], 2); } /// Review of #236: an export adding to Photos reserves the import before it writes its files. diff --git a/crates/ui-egui/locales/de.json b/crates/ui-egui/locales/de.json index bd97f5ea1..2b130f2ec 100644 --- a/crates/ui-egui/locales/de.json +++ b/crates/ui-egui/locales/de.json @@ -2043,5 +2043,6 @@ "Add to Apple Photos": "Zu Apple Fotos hinzufügen", "Adding to Apple Photos…": "Wird zu Apple Fotos hinzugefügt…", "not added to Apple Photos (cancelled)": "nicht zu Apple Fotos hinzugefügt (abgebrochen)", - "The first time, macOS asks whether LightCraft may control Photos.": "Beim ersten Mal fragt macOS, ob LightCraft Fotos steuern darf." + "The first time, macOS asks whether LightCraft may control Photos.": "Beim ersten Mal fragt macOS, ob LightCraft Fotos steuern darf.", + "Adding to Apple Photos": "Wird zu Apple Fotos hinzugefügt" } diff --git a/crates/ui-egui/locales/es.json b/crates/ui-egui/locales/es.json index c4c6e1c51..b9a3b4270 100644 --- a/crates/ui-egui/locales/es.json +++ b/crates/ui-egui/locales/es.json @@ -1888,5 +1888,6 @@ "Add to Apple Photos": "Añadir a Fotos de Apple", "Adding to Apple Photos…": "Añadiendo a Fotos de Apple…", "not added to Apple Photos (cancelled)": "no se añadieron a Fotos de Apple (cancelado)", - "The first time, macOS asks whether LightCraft may control Photos.": "La primera vez, macOS pregunta si LightCraft puede controlar Fotos." + "The first time, macOS asks whether LightCraft may control Photos.": "La primera vez, macOS pregunta si LightCraft puede controlar Fotos.", + "Adding to Apple Photos": "Añadiendo a Fotos de Apple" } diff --git a/crates/ui-egui/locales/fr.json b/crates/ui-egui/locales/fr.json index 503b8cbae..3a84f7d34 100644 --- a/crates/ui-egui/locales/fr.json +++ b/crates/ui-egui/locales/fr.json @@ -1888,5 +1888,6 @@ "Add to Apple Photos": "Ajouter à Apple Photos", "Adding to Apple Photos…": "Ajout à Apple Photos…", "not added to Apple Photos (cancelled)": "non ajouté à Apple Photos (annulé)", - "The first time, macOS asks whether LightCraft may control Photos.": "La première fois, macOS demande si LightCraft peut contrôler Photos." + "The first time, macOS asks whether LightCraft may control Photos.": "La première fois, macOS demande si LightCraft peut contrôler Photos.", + "Adding to Apple Photos": "Ajout à Apple Photos" } diff --git a/crates/ui-egui/locales/ja.json b/crates/ui-egui/locales/ja.json index b5b891d09..23c4e2c07 100644 --- a/crates/ui-egui/locales/ja.json +++ b/crates/ui-egui/locales/ja.json @@ -1890,5 +1890,6 @@ "Add to Apple Photos": "Appleの写真に追加", "Adding to Apple Photos…": "Appleの写真に追加中…", "not added to Apple Photos (cancelled)": "Appleの写真には追加されませんでした(キャンセル)", - "The first time, macOS asks whether LightCraft may control Photos.": "初回は、LightCraftに写真の操作を許可するかmacOSが確認します。" + "The first time, macOS asks whether LightCraft may control Photos.": "初回は、LightCraftに写真の操作を許可するかmacOSが確認します。", + "Adding to Apple Photos": "Appleの写真に追加中" } diff --git a/crates/ui-egui/locales/pt-br.json b/crates/ui-egui/locales/pt-br.json index 698a274df..cdbf23f4e 100644 --- a/crates/ui-egui/locales/pt-br.json +++ b/crates/ui-egui/locales/pt-br.json @@ -1888,5 +1888,6 @@ "Add to Apple Photos": "Adicionar ao Fotos da Apple", "Adding to Apple Photos…": "Adicionando ao Fotos da Apple…", "not added to Apple Photos (cancelled)": "não adicionadas ao Fotos da Apple (cancelado)", - "The first time, macOS asks whether LightCraft may control Photos.": "Na primeira vez, o macOS pergunta se o LightCraft pode controlar o Fotos." + "The first time, macOS asks whether LightCraft may control Photos.": "Na primeira vez, o macOS pergunta se o LightCraft pode controlar o Fotos.", + "Adding to Apple Photos": "Adicionando ao Fotos da Apple" } diff --git a/crates/ui-egui/locales/ru.json b/crates/ui-egui/locales/ru.json index 364f07ba5..23963fdc3 100644 --- a/crates/ui-egui/locales/ru.json +++ b/crates/ui-egui/locales/ru.json @@ -1602,5 +1602,6 @@ "Add to Apple Photos": "Добавить в Apple Фото", "Adding to Apple Photos…": "Добавление в Apple Фото…", "not added to Apple Photos (cancelled)": "не добавлено в Apple Фото (отменено)", - "The first time, macOS asks whether LightCraft may control Photos.": "В первый раз macOS спросит, может ли LightCraft управлять приложением «Фото»." + "The first time, macOS asks whether LightCraft may control Photos.": "В первый раз macOS спросит, может ли LightCraft управлять приложением «Фото».", + "Adding to Apple Photos": "Добавление в Apple Фото" } diff --git a/crates/ui-egui/locales/uk.json b/crates/ui-egui/locales/uk.json index b76dea8f2..8df7036ce 100644 --- a/crates/ui-egui/locales/uk.json +++ b/crates/ui-egui/locales/uk.json @@ -2082,5 +2082,6 @@ "Add to Apple Photos": "Додати до Apple Фото", "Adding to Apple Photos…": "Додавання до Apple Фото…", "not added to Apple Photos (cancelled)": "не додано до Apple Фото (скасовано)", - "The first time, macOS asks whether LightCraft may control Photos.": "Першого разу macOS запитає, чи може LightCraft керувати програмою «Фото»." + "The first time, macOS asks whether LightCraft may control Photos.": "Першого разу macOS запитає, чи може LightCraft керувати програмою «Фото».", + "Adding to Apple Photos": "Додавання до Apple Фото" } diff --git a/crates/ui-egui/locales/zh-hans.json b/crates/ui-egui/locales/zh-hans.json index b38b91b29..a82443bca 100644 --- a/crates/ui-egui/locales/zh-hans.json +++ b/crates/ui-egui/locales/zh-hans.json @@ -1888,5 +1888,6 @@ "Add to Apple Photos": "添加到 Apple 照片", "Adding to Apple Photos…": "正在添加到 Apple 照片…", "not added to Apple Photos (cancelled)": "未添加到 Apple 照片(已取消)", - "The first time, macOS asks whether LightCraft may control Photos.": "首次使用时,macOS 会询问是否允许 LightCraft 控制“照片”。" + "The first time, macOS asks whether LightCraft may control Photos.": "首次使用时,macOS 会询问是否允许 LightCraft 控制“照片”。", + "Adding to Apple Photos": "正在添加到 Apple 照片" } diff --git a/crates/ui-egui/locales/zh-hant.json b/crates/ui-egui/locales/zh-hant.json index 073d9f8be..41d50eea6 100644 --- a/crates/ui-egui/locales/zh-hant.json +++ b/crates/ui-egui/locales/zh-hant.json @@ -1888,5 +1888,6 @@ "Add to Apple Photos": "加入 Apple 照片", "Adding to Apple Photos…": "正在加入 Apple 照片…", "not added to Apple Photos (cancelled)": "未加入 Apple 照片(已取消)", - "The first time, macOS asks whether LightCraft may control Photos.": "第一次使用時,macOS 會詢問是否允許 LightCraft 控制「照片」。" + "The first time, macOS asks whether LightCraft may control Photos.": "第一次使用時,macOS 會詢問是否允許 LightCraft 控制「照片」。", + "Adding to Apple Photos": "正在加入 Apple 照片" } diff --git a/crates/ui-egui/src/export_task.rs b/crates/ui-egui/src/export_task.rs index b6de2f41b..051ad22bd 100644 --- a/crates/ui-egui/src/export_task.rs +++ b/crates/ui-egui/src/export_task.rs @@ -22,8 +22,9 @@ pub struct ExportTask { /// Photos started so far and the file in progress. pub progress: Arc>, pub cancel: Arc, - /// The files (per-photo results) and what [`AfterExport`] did with them (`applePhotos`). - rx: Receiver, Option), String>>, + /// The files (per-photo results), what [`AfterExport`] did with them (`applePhotos`) and + /// whether the export was cancelled (as the worker saw it when the batch ended). + rx: Receiver, Option, bool), String>>, /// The export's row in the activity stack (`activity.cancel` sets `cancel`). guard: TaskGuard, } @@ -48,6 +49,9 @@ pub fn start(app: &mut LightcraftApp, items: Vec, opts: ExportOp let cancel = Arc::new(AtomicBool::new(false)); let (tx, rx) = channel(); let (p, c) = (progress.clone(), cancel.clone()); + let guard = app.session.activity.start("export", "Exporting", Cancel::Flag(cancel.clone())); + guard.progress(0, total as u64); + let row = guard.handle(); // translated here: the language is the UI thread's, and the worker would show English let adding = crate::i18n::tr("Adding to Apple Photos…").to_string(); let work = move || { @@ -59,13 +63,17 @@ pub fn start(app: &mut LightcraftApp, items: Vec, opts: ExportOp !c.load(Ordering::Relaxed) }); let r = r.map(|files| { - if !after.is_none() - && let Ok(mut g) = p.lock() - { - g.1 = adding; + if !after.is_none() { + // Photos can't be stopped once asked: no ✕ from here, before the last look at + // the cancel flag, so a cancel either stops the export before Photos or is refused + row.set_cancellable(false); + if let Ok(mut g) = p.lock() { + g.1 = adding; + } } - let photos = after.run(&files, c.load(Ordering::Relaxed)); - (files, photos) + let cancelled = c.load(Ordering::Relaxed); + let photos = after.run(&files, cancelled); + (files, photos, cancelled) }); let _ = tx.send(r); }; @@ -73,8 +81,6 @@ pub fn start(app: &mut LightcraftApp, items: Vec, opts: ExportOp std::thread::Builder::new().name("export".into()).spawn(work).map_err(|e| e.to_string())?; #[cfg(target_arch = "wasm32")] work(); - let guard = app.session.activity.start("export", "Exporting", Cancel::Flag(cancel.clone())); - guard.progress(0, total as u64); app.export = Some(ExportTask { total, progress, cancel, rx, guard }); Ok(json!({"background": true, "total": total})) } @@ -114,7 +120,7 @@ pub fn start_contact_sheet(app: &mut LightcraftApp, params: &Value) -> Result { - let cancelled = task.cancel.load(Ordering::Relaxed); + let cancelled = matches!(&r, Ok((_, _, true))); let total = task.total; app.export = None; // Photos refused (permission, not installed…): leave the way out on screen longer - let photos_failed = matches!(&r, Ok((_, Some(p))) if p.get("error").is_some()); + let photos_failed = matches!(&r, Ok((_, Some(p), _)) if p.get("error").is_some()); let msg = match r { - Ok((files, photos)) => { + Ok((files, photos, _)) => { let sheet = files.first().filter(|f| f["contactSheet"] == true); let ok = files.iter().filter(|f| f.get("path").is_some()).count(); let failed: Vec<&Value> = files.iter().filter(|f| f.get("error").is_some()).collect(); diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 1bf180217..b0cdc4657 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -1885,7 +1885,10 @@ mod tests { h.app.services.write_shared = Some(Arc::new(|p: &str, bytes: &[u8]| lightcraft_engine::export::write_file(p, bytes))); let (release, held) = std::sync::mpsc::channel::<()>(); let held = std::sync::Mutex::new(held); + let (entered, photos_asked) = std::sync::mpsc::channel::<()>(); + let entered = std::sync::Mutex::new(entered); h.app.session.apple_photos = Some(Arc::new(move |_: &[String], _| { + let _ = entered.lock().unwrap().send(()); let _ = held.lock().unwrap().recv_timeout(Duration::from_secs(30)); Ok(lightcraft_engine::apple_photos::Output { status: Some(0), stdout: "ID\n".into(), stderr: String::new() }) })); @@ -1893,19 +1896,25 @@ mod tests { let params = json!({"ids": ids, "dir": out, "longEdge": 48, "addToPhotos": true, "background": true}); let r = h.request("engine.execute", json!({"command": "app.export", "params": params}), t); assert_eq!(r["ok"], true, "{r}"); - let t0 = Instant::now(); - let current = loop { - h.step(); - let current = h.app.export.as_ref().map(|e| e.progress.lock().unwrap().1.clone()).unwrap_or_default(); - if current.contains("Apple") || t0.elapsed() > Duration::from_secs(60) { - break current; - } - }; + // Photos has been asked, and the window hasn't drawn a frame since (Codex review of #643): + // the export's row already has no ✕, and a cancel is refused rather than half-honoured + photos_asked.recv_timeout(Duration::from_secs(60)).expect("the export asks Photos"); + let row = h.app.session.activity.list().into_iter().find(|r| r.kind == "export").expect("the export's row"); + assert!(!row.cancellable, "{row:?}"); + assert!(h.app.session.activity.cancel(row.id).is_err(), "Photos can't be stopped once asked"); + let current = h.app.export.as_ref().map(|e| e.progress.lock().unwrap().1.clone()).unwrap_or_default(); assert_eq!(current, "Wird zu Apple Fotos hinzugefügt…"); + // the row says so on the next frame (issue #345) + h.step(); + let row = h.app.session.activity.list().into_iter().find(|r| r.kind == "export").expect("the export's row"); + assert_eq!((row.detail.as_str(), row.cancellable), ("Wird zu Apple Fotos hinzugefügt…", false), "{row:?}"); release.send(()).unwrap(); + let t0 = Instant::now(); while h.app.export.is_some() && t0.elapsed() < Duration::from_secs(90) { h.step(); } + let last = h.app.last_export_result.clone().expect("the export reports"); + assert_eq!((last["cancelled"].as_bool(), last["applePhotos"]["imported"].as_u64()), (Some(false), Some(1)), "{last}"); crate::i18n::set_language(crate::i18n::Locale::En); let _ = std::fs::remove_dir_all(&out); } diff --git a/docs/apple-photos.md b/docs/apple-photos.md index 1fc50f654..f0d47c5e3 100644 --- a/docs/apple-photos.md +++ b/docs/apple-photos.md @@ -29,11 +29,17 @@ An import can take minutes, and the first one waits for the user to answer macOS import is a job: `{job, running, requested, album}`, plus, once Photos is done, `{imported, ids, warning?}`, `{error}` or `{skipped}` (`exporting: true` while an export that will add its files is still writing them). +While Photos works, the activity stack ([background-tasks.md](background-tasks.md)) shows it. An export adding its +files keeps its own row, which reads "Adding to Apple Photos…" and loses its ✕, since Photos can't be stopped once +asked. An import of its own (`export.addToPhotos`, or the Photos step left running after `app.export` without +`background`) gets a row of kind `applePhotos`, "Adding to Apple Photos", with the album as its detail, no ✕ and no +count, as Photos reports none until it is done. + - In the **desktop app** nothing waits for Photos on the window's thread. The Export dialog's background export adds the files on its worker and reports them in its toast. `app.export` without `background` and `export.addToPhotos` answer at once with the running job (`running: true`); the app shows a toast when Photos - is done, and `export.photosImports {job}` gives the outcome. (`export.addToPhotos {wait: true}` waits there - too, holding the window until Photos is done.) + is done, and `export.photosImports {job}` gives the outcome. (`export.addToPhotos {wait: true}` is refused there: + the window's thread never waits for Photos.) - **Without a window** (the MCP server, `lightcraft-cli`, the headless snapshot) everything waits for Photos by default: `app.export` and `export.addToPhotos` return the finished job, so a one-shot command never ends in the middle of an import. `export.addToPhotos {wait: false}` returns the running job instead (for a diff --git a/docs/mcp.md b/docs/mcp.md index 01fdcf9ef..2c9b30ed3 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -278,7 +278,7 @@ plus `{imported, ids, warning?}`, `{error}` or `{skipped}` once Photos is done; way. Headless (this server, `lightcraft-cli`) waits for Photos by default; the desktop app (connect mode) answers with the running job and `export.photosImports {job}` gives the outcome. `export.addToPhotos {paths, album?, wait?}` does the same for files already on disk (`wait` defaults to true headless, false in the desktop -app; with `wait: false` headless, the server waits for the import before it exits at end of input). One import +app, which refuses `wait: true`; with `wait: false` headless, the server waits for the import before it exits at end of input). One import runs at a time, and an export adding to Photos reserves it before it writes: another is refused, naming the job in the way. On other platforms `addToPhotos: true` is refused before anything is written, and `export.addToPhotos` is disabled with the reason. The first time, macOS asks the user to allow LightCraft (or diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index cce814bd6..7ec59c5b6 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -53,9 +53,9 @@ cited here still exists. Formerly docs/parity.md. | Y. Menus | 82 | 0 | 4 | 8 | 47/47 (100%) | 23/23 (100%) | | Z. Keyboard shortcuts (desktop) | 75 | 3 | 3 | 1 | 49/52 (94%) | 23/24 (96%) | | Lightroom Classic extras | 33 | 15 | 35 | 9 | — | 23/25 (92%) | -| **Total** | 397 | 49 | 73 | 36 | 192/200 (96%) | 143/155 (92%) | +| **Total** | 397 | 50 | 72 | 36 | 192/200 (96%) | 143/155 (92%) | -Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **81.2%** of 519 in-scope rows — P0 98.0% of 200 · P1 95.2% of 155 · P2 47.2% of 163. +Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **81.3%** of 519 in-scope rows — P0 98.0% of 200 · P1 95.2% of 155 · P2 47.5% of 163. ## Top gaps and shortcut notes