From 484c160c13a43025bbb328903e5bdb6f98858119 Mon Sep 17 00:00:00 2001 From: Alex Chambers Date: Sat, 10 Oct 2026 12:11:20 +0200 Subject: [PATCH] LR-EXP-PHOTOS: Apple Photos imports in the activity stack (#345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto the activity stack (#601). An export adding its files to Photos keeps its row through that step, with "Adding to Apple Photos…" as the detail. The worker turns the row's ✕ off (TaskHandle::set_cancellable) before its last look at the cancel flag and reports what it saw, so a cancel either stops the export before Photos is asked or is refused, never both "cancelled" and imported. activity: a per-task gate orders a cancel's decision against a step switching cancelling off, so a cancel can't be accepted between the switch and the step's look at the flag and then be ignored. An import of its own (export.addToPhotos, or the Photos step left running after a foreground app.export) holds a row of kind applePhotos while Photos works: no ✕, no count, the album as detail. The desktop app refuses export.addToPhotos {wait: true}: its window's thread never waits for Photos. The label is in every catalog; tests assert the rows, the refused cancel and the refused wait. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/activity.rs | 81 ++++++++++++---- crates/engine/src/apple_photos.rs | 142 +++++++++++++++++++++++----- crates/engine/src/cmd/export.rs | 11 ++- crates/engine/src/export.rs | 4 +- crates/engine/src/tests_export.rs | 5 +- crates/ui-egui/locales/de.json | 3 +- crates/ui-egui/locales/es.json | 3 +- crates/ui-egui/locales/fr.json | 3 +- crates/ui-egui/locales/ja.json | 3 +- crates/ui-egui/locales/pt-br.json | 3 +- crates/ui-egui/locales/ru.json | 3 +- crates/ui-egui/locales/uk.json | 3 +- crates/ui-egui/locales/zh-hans.json | 3 +- crates/ui-egui/locales/zh-hant.json | 3 +- crates/ui-egui/src/export_task.rs | 34 ++++--- crates/ui-egui/src/headless.rs | 25 +++-- docs/apple-photos.md | 10 +- docs/mcp.md | 2 +- docs/parity-checklist.md | 4 +- 19 files changed, 266 insertions(+), 79 deletions(-) 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