LR-EXP-PHOTOS: Apple Photos fixes from the live test and review
- Ten minutes plus five seconds a file per import (was one minute): the first live run timed out while macOS's permission prompt was still up, and Photos' duplicates question waits for a person too. - Files go to osascript 200 at a time, all within the import's one time limit: one run with every path went over macOS's 1 MiB argument limit at about 10,000 files, and Photos got nothing. A failed later batch says how many files Photos had added. - The progress line "Adding to Apple Photos…" is translated on the UI thread; on the export worker it was always English. - docs/apple-photos.md: who can add what (the control port is reachable by any local program, other users' included), batching, the new limit, and tested end to end (macOS 26.6.2, Photos 11.0, packaged app). The parity row says so; What's New has an entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Alex Chambers
parent
a6431c152d
commit
c722e6bb64
|
|
@ -203,10 +203,12 @@ pub fn explain_run_error(e: &RunError) -> String {
|
|||
}
|
||||
}
|
||||
|
||||
/// How long one import may take: a minute, plus five seconds a file, at most two hours (the first
|
||||
/// import also waits for the user to answer the permission prompt).
|
||||
/// How long one import may take: ten minutes, plus five seconds a file, at most two hours. The ten
|
||||
/// minutes are for a person: the first import waits for the user to answer macOS's permission
|
||||
/// prompt, and Photos may ask about duplicates; a first try with one minute timed out while the
|
||||
/// prompt was still up.
|
||||
pub fn timeout_for(files: usize) -> Duration {
|
||||
Duration::from_secs(60u64.saturating_add(5u64.saturating_mul(files as u64)).min(7200))
|
||||
Duration::from_secs(600u64.saturating_add(5u64.saturating_mul(files as u64)).min(7200))
|
||||
}
|
||||
|
||||
/// What an import gave.
|
||||
|
|
@ -258,13 +260,37 @@ pub fn check(paths: &[String], album: &str) -> Result<String, String> {
|
|||
Ok(album.to_string())
|
||||
}
|
||||
|
||||
/// Run osascript for a checked request (`album` trimmed; see [`check`]).
|
||||
/// Files per osascript run. The paths are arguments, and macOS limits a process's arguments to
|
||||
/// 1 MiB in all: 200 paths of the longest length macOS opens (1,024 bytes; [`check`] finds no
|
||||
/// file at a longer one) stay below a quarter of that.
|
||||
pub const BATCH: usize = 200;
|
||||
|
||||
/// Run osascript for a checked request (`album` trimmed; see [`check`]): [`BATCH`] files at a
|
||||
/// time, all within [`timeout_for`] the whole request. The first run makes the album when it is
|
||||
/// missing; the others find it.
|
||||
fn execute(runner: &Runner, paths: &[String], album: &str) -> Result<Imported, String> {
|
||||
let out = runner(&osascript_args(album, paths), timeout_for(paths.len())).map_err(|e| explain_run_error(&e))?;
|
||||
if out.status != Some(0) {
|
||||
return Err(explain(&out, album));
|
||||
let limit = timeout_for(paths.len());
|
||||
let deadline = std::time::Instant::now() + limit;
|
||||
let mut ids = Vec::new();
|
||||
for (i, batch) in paths.chunks(BATCH).enumerate() {
|
||||
let left = deadline.saturating_duration_since(std::time::Instant::now());
|
||||
let out = if left.is_zero() { Err(RunError::Timeout(limit)) } else { runner(&osascript_args(album, batch), left) };
|
||||
let failed = match out {
|
||||
Ok(out) if out.status == Some(0) => {
|
||||
ids.extend(parse_ids(&out.stdout));
|
||||
continue;
|
||||
}
|
||||
Ok(out) => explain(&out, album),
|
||||
// the request's limit, not what was left of it for this batch
|
||||
Err(RunError::Timeout(_)) => explain_run_error(&RunError::Timeout(limit)),
|
||||
Err(e) => explain_run_error(&e),
|
||||
};
|
||||
if i == 0 {
|
||||
return Err(failed);
|
||||
}
|
||||
return Err(format!("{failed} (Photos had added {} of the {} files before that.)", ids.len(), paths.len()));
|
||||
}
|
||||
Ok(Imported { ids: parse_ids(&out.stdout), requested: paths.len(), album: (!album.is_empty()).then(|| album.to_string()) })
|
||||
Ok(Imported { ids, requested: paths.len(), album: (!album.is_empty()).then(|| album.to_string()) })
|
||||
}
|
||||
|
||||
/// Add `paths` (absolute paths of existing files) to Photos, into `album` when it isn't blank,
|
||||
|
|
@ -845,11 +871,68 @@ mod tests {
|
|||
// a failed run is the mapped message
|
||||
let (runner, _) = fake(Ok(failed("execution error: Not authorized to send Apple events to Photos. (-1743)")));
|
||||
assert!(import(&runner, &paths, "x").unwrap_err().contains("Automation"));
|
||||
// a timeout names the request's limit
|
||||
let (runner, _) = fake(Err(RunError::Timeout(Duration::from_secs(70))));
|
||||
assert!(import(&runner, &paths, "x").unwrap_err().contains("70 s"));
|
||||
assert!(import(&runner, &paths, "x").unwrap_err().contains(&format!("{} s", timeout_for(2).as_secs())));
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
}
|
||||
|
||||
/// Codex review of #643: every path was an argument of one osascript run, so a big export
|
||||
/// (10,000 files) went over macOS's 1 MiB argument limit and Photos got nothing.
|
||||
#[test]
|
||||
fn big_imports_run_in_batches_within_the_argument_limit() {
|
||||
let long = format!("/{}", "d/".repeat(500)); // about 1,000 bytes, near the longest path macOS opens
|
||||
let paths: Vec<String> = (0..10_000).map(|i| format!("{long}{i:05}.jpg")).collect();
|
||||
let runs = Arc::new(Mutex::new(Vec::<(usize, usize, Duration)>::new()));
|
||||
let r = runs.clone();
|
||||
let runner: Runner = Arc::new(move |args: &[String], limit| {
|
||||
let files = args.len() - args.iter().position(|a| a == MARKER).unwrap() - 2;
|
||||
r.lock().unwrap().push((files, args.iter().map(|a| a.len() + 1).sum(), limit));
|
||||
Ok(Output { status: Some(0), stdout: (0..files).map(|i| format!("ID-{i}\n")).collect(), stderr: String::new() })
|
||||
});
|
||||
let got = execute(&runner, &paths, "Big").unwrap();
|
||||
assert_eq!((got.ids.len(), got.requested, got.warning()), (10_000, 10_000, None));
|
||||
let runs = runs.lock().unwrap().clone();
|
||||
assert_eq!(runs.len(), 10_000 / BATCH);
|
||||
assert!(runs.iter().all(|&(files, _, _)| files == BATCH));
|
||||
let biggest = runs.iter().map(|&(_, bytes, _)| bytes).max().unwrap();
|
||||
assert!(biggest < 1 << 18, "{biggest} bytes of arguments in one run");
|
||||
// every run gets what is left of the request's limit, never more
|
||||
assert!(runs.iter().all(|&(_, _, limit)| limit <= timeout_for(10_000)));
|
||||
|
||||
// a later batch failing: the error says how far Photos got
|
||||
let calls = Arc::new(Mutex::new(0));
|
||||
let c = calls.clone();
|
||||
let runner: Runner = Arc::new(move |args: &[String], _| {
|
||||
let mut n = c.lock().unwrap();
|
||||
*n += 1;
|
||||
let files = args.len() - args.iter().position(|a| a == MARKER).unwrap() - 2;
|
||||
if *n == 2 {
|
||||
return Ok(failed("execution error: Photos got an error: User canceled. (-128)"));
|
||||
}
|
||||
Ok(Output { status: Some(0), stdout: (0..files).map(|i| format!("ID-{i}\n")).collect(), stderr: String::new() })
|
||||
});
|
||||
let e = execute(&runner, &paths[..450], "Big").unwrap_err();
|
||||
assert!(e.contains("cancelled") && e.contains("200 of the 450 files"), "{e}");
|
||||
assert_eq!(*calls.lock().unwrap(), 2, "nothing more is sent after a failed batch");
|
||||
|
||||
// the batches share the request's one deadline: time a batch takes comes off the next one's
|
||||
let limits = Arc::new(Mutex::new(Vec::<Duration>::new()));
|
||||
let l = limits.clone();
|
||||
let runner: Runner = Arc::new(move |args: &[String], limit| {
|
||||
l.lock().unwrap().push(limit);
|
||||
std::thread::sleep(Duration::from_millis(300));
|
||||
let files = args.len() - args.iter().position(|a| a == MARKER).unwrap() - 2;
|
||||
Ok(Output { status: Some(0), stdout: (0..files).map(|i| format!("ID-{i}\n")).collect(), stderr: String::new() })
|
||||
});
|
||||
execute(&runner, &paths[..450], "Big").unwrap();
|
||||
let limits = limits.lock().unwrap().clone();
|
||||
assert_eq!(limits.len(), 3);
|
||||
let whole = timeout_for(450);
|
||||
assert!(limits[0] <= whole && limits[0] > whole - Duration::from_millis(250), "{limits:?}");
|
||||
assert!(limits[1] <= whole - Duration::from_millis(300) && limits[2] <= whole - Duration::from_millis(600), "{limits:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bad_requests_never_reach_osascript() {
|
||||
let (dir, paths) = files("bad", 1);
|
||||
|
|
@ -1106,8 +1189,8 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn timeouts_grow_with_the_batch_and_stay_bounded() {
|
||||
assert_eq!(timeout_for(1), Duration::from_secs(65));
|
||||
assert_eq!(timeout_for(100), Duration::from_secs(560));
|
||||
assert_eq!(timeout_for(1), Duration::from_secs(605));
|
||||
assert_eq!(timeout_for(100), Duration::from_secs(1100));
|
||||
assert_eq!(timeout_for(usize::MAX), Duration::from_secs(7200));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -48,6 +48,8 @@ pub fn start(app: &mut LightcraftApp, items: Vec<PreparedExport>, opts: ExportOp
|
|||
let cancel = Arc::new(AtomicBool::new(false));
|
||||
let (tx, rx) = channel();
|
||||
let (p, c) = (progress.clone(), cancel.clone());
|
||||
// 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 || {
|
||||
let mut w = |path: &str, bytes: &[u8]| write(path, bytes);
|
||||
let r = run_batch(items, &opts, &to, &mut w, &|path| std::path::Path::new(path).exists(), false, &mut |done, name| {
|
||||
|
|
@ -60,7 +62,7 @@ pub fn start(app: &mut LightcraftApp, items: Vec<PreparedExport>, opts: ExportOp
|
|||
if !after.is_none()
|
||||
&& let Ok(mut g) = p.lock()
|
||||
{
|
||||
g.1 = crate::i18n::tr("Adding to Apple Photos…").to_string();
|
||||
g.1 = adding;
|
||||
}
|
||||
let photos = after.run(&files, c.load(Ordering::Relaxed));
|
||||
(files, photos)
|
||||
|
|
|
|||
|
|
@ -1869,6 +1869,47 @@ mod tests {
|
|||
let _ = std::fs::remove_dir_all(&out);
|
||||
}
|
||||
|
||||
/// Codex review of #643: the progress line while Photos imports was translated on the export's
|
||||
/// worker thread, whose language is always English.
|
||||
#[test]
|
||||
fn the_photos_progress_line_is_in_the_users_language() {
|
||||
if !cfg!(target_os = "macos") {
|
||||
return;
|
||||
}
|
||||
let mut h = demo([1200.0, 900.0]);
|
||||
let t = Duration::from_secs(10);
|
||||
let out = std::env::temp_dir().join(format!("lc-photos-language-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&out);
|
||||
h.app.ui.language = crate::i18n::Locale::De;
|
||||
crate::i18n::set_language(crate::i18n::Locale::De);
|
||||
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);
|
||||
h.app.session.apple_photos = Some(Arc::new(move |_: &[String], _| {
|
||||
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() })
|
||||
}));
|
||||
let ids: Vec<u64> = h.app.session.visible_cloned().iter().take(1).map(|p| p.0).collect();
|
||||
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;
|
||||
}
|
||||
};
|
||||
assert_eq!(current, "Wird zu Apple Fotos hinzugefügt…");
|
||||
release.send(()).unwrap();
|
||||
while h.app.export.is_some() && t0.elapsed() < Duration::from_secs(90) {
|
||||
h.step();
|
||||
}
|
||||
crate::i18n::set_language(crate::i18n::Locale::En);
|
||||
let _ = std::fs::remove_dir_all(&out);
|
||||
}
|
||||
|
||||
/// Issue #236 review: control requests run on the UI thread, which must not wait minutes for
|
||||
/// Photos. `export.addToPhotos` and `app.export {addToPhotos}` without `background` answer at
|
||||
/// once with the running job; the app announces the outcome when Photos is done.
|
||||
|
|
|
|||
|
|
@ -4,9 +4,9 @@ An export can hand the files it wrote to Apple Photos, optionally into an album
|
|||
`LR-EXP-PHOTOS`). macOS only: on other platforms the option is hidden in the Export dialog and refused by
|
||||
`app.export`. The web build doesn't include it.
|
||||
|
||||
> **Status: not yet tested end to end.** The code and its tests (which use a stand-in for osascript) are in
|
||||
> place, but no import into a real Photos library has been run yet, and the AppleScript has never been run.
|
||||
> Please try the [manual test](#manual-test) below, with the packaged app, and report what happens.
|
||||
> **Tested end to end** on macOS 26.6.2 with Photos 11.0, using the ad-hoc-signed packaged app and a new, empty
|
||||
> Photos library: the [manual test](#manual-test) below, except the two `lightcraft-cli` steps (11 and 12). Please
|
||||
> run it again after changing the script or the packaging.
|
||||
|
||||
## Using it
|
||||
|
||||
|
|
@ -16,8 +16,9 @@ An export can hand the files it wrote to Apple Photos, optionally into an album
|
|||
- **Export with Previous** and saved export presets repeat it (the setting is part of the export options).
|
||||
- **Agents and scripts:** `app.export {…, addToPhotos: true, photosAlbum?: "Name"}` (desktop app, MCP `export`
|
||||
tool, `lightcraft-cli run`); `lightcraft-cli render in.dng -o out.jpg --opt addToPhotos=true --opt
|
||||
photosAlbum=Name`. The export's result carries `applePhotos` (below). A failed Photos step does not fail
|
||||
the export: the files are written either way.
|
||||
photosAlbum=Name`. The export's result carries `applePhotos` (below). The files are written either way: a
|
||||
failed Photos step doesn't fail `app.export` (its `applePhotos` has the `error`), while `lightcraft-cli render`
|
||||
says the file was written and exits with the error.
|
||||
- **Files already on disk:** `export.addToPhotos {paths: [absolute paths], album?, wait?}`.
|
||||
- **How an import went:** `export.photosImports` lists this session's imports, oldest first;
|
||||
`export.photosImports {job}` gives one.
|
||||
|
|
@ -65,7 +66,9 @@ Settings ▸ General ▸ *Copy items to the Photos library*. When it references
|
|||
LightCraft runs `/usr/bin/osascript` with a constant AppleScript (`SCRIPT` in
|
||||
`crates/engine/src/apple_photos.rs`) that asks Photos, through its scripting interface, to import the files:
|
||||
`import … into album … skip check duplicates false`. The album is found or made first ([Which
|
||||
album](#which-album)). The script returns the new media items' ids.
|
||||
album](#which-album)). The script returns the new media items' ids. Files go to Photos 200 at a time (each path is an osascript argument, and
|
||||
macOS limits a program's arguments to 1 MiB), one run after another within the import's time limit; the first
|
||||
run makes the album when it is missing.
|
||||
|
||||
This is Apple Events to Photos' own import command. It is not the OS-level synthetic input that AGENTS.md
|
||||
forbids: nothing is typed or clicked, and no other window is touched.
|
||||
|
|
@ -91,8 +94,9 @@ not only files it exported. So any client of the control port or the MCP server
|
|||
video on the Mac into the user's Photos library, and from there into iCloud Photos when that is on. macOS asks
|
||||
for the Automation permission once per app, not per import, so after the first Allow nothing asks again. The
|
||||
control port listens on 127.0.0.1 only and has no authentication ([control-protocol.md](control-protocol.md)),
|
||||
and the MCP server answers the program that started it, so this reaches programs already running as the user.
|
||||
Treat giving an agent the MCP server or the control port as giving it this too.
|
||||
so any program on this Mac that can connect to it can do this, including programs of other users logged in to
|
||||
the Mac. The MCP server talks only to the program that started it (stdio). Treat giving an agent the MCP server
|
||||
or the control port as giving it this too, and turn the control port on only when you need it.
|
||||
|
||||
## When it doesn't work
|
||||
|
||||
|
|
@ -102,7 +106,7 @@ Treat giving an agent the MCP server or the control port as giving it this too.
|
|||
| "Apple Photos couldn't be found…" | Photos isn't installed or couldn't be opened (`-600`, `-10810`, `-10814`, or no `/System/Applications/Photos.app`). |
|
||||
| "The import was cancelled in Photos." | Someone pressed Cancel in a Photos dialog (`-128`). |
|
||||
| "More than one album at the top level of Photos is named …" | Several top-level albums have that name; nothing was added. Rename one in Photos, or use another name. |
|
||||
| "Photos didn't finish adding the photos within N s…" / "didn't answer in time" | LightCraft waits a minute plus five seconds a file (at most two hours), then stops osascript. Photos may still be importing: look in Photos before trying again. |
|
||||
| "Photos didn't finish adding the photos within N s…" / "didn't answer in time" | LightCraft waits ten minutes plus five seconds a file (at most two hours), then stops osascript. The ten minutes leave time to answer macOS's permission prompt and Photos' duplicates question. Photos may still be importing: look in Photos before trying again. |
|
||||
| "Apple Photos is still adding N file(s) from an earlier request (import K)…" / "An export is about to add its files to Apple Photos (import K)…" | One import at a time: wait for import K (`export.photosImports {job: K}`), then try again. |
|
||||
| "Photos added 2 of 3 files…" | A partial import: Photos skipped files it found to be duplicates (it may ask first) or couldn't read. |
|
||||
| "Apple Photos is only available on macOS" | The setting came from a Mac (a preset or Export with Previous); the Export dialog drops it on other platforms. |
|
||||
|
|
|
|||
|
|
@ -397,7 +397,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts).
|
|||
| LR-EXP-PREVIOUS | Export with previous settings | P0 | ✅ | `cmd:app.exportPrevious`, `cmd:dialog.export` | last options persist in prefs.json; dialog prefilled; no named export presets yet |
|
||||
| LR-EXP-DNGOPT | DNG options | P2 | 🟡 | `cmd:app.export` (`dngCompression`), `crates/engine/src/export.rs` | compression: lossless JPEG (default), ZIP or none; no embedded JPEG preview size, no lossy DNG output |
|
||||
| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`), `crates/engine/src/tests_export.rs` | file copied byte for byte, sidecar named after the output and subject to the conflict policy |
|
||||
| LR-EXP-PHOTOS | Export to the system photo library | P2 | 🟡 | `cmd:app.export` (`addToPhotos`, `photosAlbum`), `cmd:export.addToPhotos`, `cmd:export.photosImports`, `crates/engine/src/apple_photos.rs`, `crates/ui-egui/src/panels/dialogs.rs`, `packaging/macos/entitlements.plist`, `docs/apple-photos.md` | macOS: the exported files go to Apple Photos through Apple Events to its import command (a constant AppleScript run by `/usr/bin/osascript`; album names and paths are separate arguments), into a top-level album of exactly that name, made when missing (several of that name: refused); the Export dialog's Add to Apple Photos, Export with Previous, presets, MCP and `lightcraft-cli` all reach it; imports are jobs, off the UI thread in the desktop app and awaited in one-shot headless runs, one at a time, reserved by an export before it writes; permission, missing Photos, timeouts and partial imports are reported; the packaged app carries the Apple Events entitlement and usage text. **Not yet tested end to end** against a real Photos library (manual steps, with the packaged app, in `docs/apple-photos.md`). Other platforms' photo libraries are not covered |
|
||||
| LR-EXP-PHOTOS | Export to the system photo library | P2 | 🟡 | `cmd:app.export` (`addToPhotos`, `photosAlbum`), `cmd:export.addToPhotos`, `cmd:export.photosImports`, `crates/engine/src/apple_photos.rs`, `crates/ui-egui/src/panels/dialogs.rs`, `packaging/macos/entitlements.plist`, `docs/apple-photos.md` | macOS: the exported files go to Apple Photos through Apple Events to its import command (a constant AppleScript run by `/usr/bin/osascript`; album names and paths are separate arguments), into a top-level album of exactly that name, made when missing (several of that name: refused); the Export dialog's Add to Apple Photos, Export with Previous, presets, MCP and `lightcraft-cli` all reach it; imports are jobs, off the UI thread in the desktop app and awaited in one-shot headless runs, one at a time, reserved by an export before it writes; permission, missing Photos, timeouts and partial imports are reported; the packaged app carries the Apple Events entitlement and usage text. Tested end to end with the packaged app on macOS 26.6.2 / Photos 11.0 (manual steps in `docs/apple-photos.md`). Other platforms' photo libraries are not covered |
|
||||
| LR-EXP-PSD | Round trip to an external editor | P2 | ✅ | `cmd:photo.editExternal`, `cmd:photo.editInExternal`, `crates/engine/src/cmd/convert.rs` | a 16-bit TIFF `-Edit` copy with the edits (Adobe RGB / ProPhoto / P3 / sRGB) next to the original, added stacked on top of it and opened in the editor set in Settings ▸ General (or the system default) |
|
||||
|
||||
## T. Share (SHARE)
|
||||
|
|
|
|||
|
|
@ -2,6 +2,15 @@
|
|||
|
||||
## 2026-10-10
|
||||
|
||||
### Add to Apple Photos (macOS)
|
||||
- The Export dialog has **Add to Apple Photos**, with an optional album. The files are exported to your folder as
|
||||
usual and then added to Photos, into a top-level album of that name (made when it's missing). The toast says how
|
||||
many Photos added, or why it added none. Export with Previous and export presets repeat it.
|
||||
- The first time, macOS asks whether LightCraft may control Photos. Photos may ask about duplicates; files it skips
|
||||
show as a partial import.
|
||||
- Agents: `app.export {addToPhotos, photosAlbum}`, `export.addToPhotos {paths}` for files already on disk, and
|
||||
`export.photosImports`. See [Add to Apple Photos](apple-photos.md).
|
||||
|
||||
### Activity stack
|
||||
- Long-running tasks show in one place, top left under the top bar, as in Lightroom Classic (issue #345): imports and
|
||||
folder scans, exports (and the contact sheet PDF), Synchronize Folder, Build / Smart Previews, Lightroom catalog import, Photo Merge, model downloads, the face scan,
|
||||
|
|
|
|||
Loading…
Reference in New Issue