diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 1109859e..8eb4c010 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -374,25 +374,14 @@ fn probe_all(probe: Option<&crate::media::FileProbe>, paths: &[String], progress } /// Copy `src` into `root`/`folders` as `name` (default: its own name), made unique with -1, -2…; -/// returns the new path. +/// returns the new path. The copy is verified ([`crate::import_move::copy_new`]: a new file, +/// synced and compared byte for byte); a bad one is removed and the photo reported as failed. fn copy_into(root: &Path, src: &str, folders: &[String], name: Option<&str>) -> Result { let dir = folders.iter().fold(root.to_path_buf(), |d, f| d.join(f)); - std::fs::create_dir_all(&dir).map_err(|e| format!("{}: {e}", dir.display()))?; let name = name .map(str::to_string) .unwrap_or_else(|| Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "photo".into())); - let (stem, ext) = match name.rsplit_once('.') { - Some((s, e)) => (s.to_string(), format!(".{e}")), - None => (name.clone(), String::new()), - }; - let mut dst = dir.join(&name); - let mut i = 1; - while dst.exists() { - dst = dir.join(format!("{stem}-{i}{ext}")); - i += 1; - } - std::fs::copy(src, &dst).map_err(|e| format!("copy {src}: {e}"))?; - Ok(dst.to_string_lossy().to_string()) + crate::import_move::copy_new(Path::new(src), &dir, &name).map(|p| p.to_string_lossy().to_string()) } /// What a [`scan`] needs from the session, so it can run on another thread (a folder on a network diff --git a/crates/engine/src/import_move.rs b/crates/engine/src/import_move.rs index 3c949e79..a32865b2 100644 --- a/crates/engine/src/import_move.rs +++ b/crates/engine/src/import_move.rs @@ -169,6 +169,27 @@ pub(crate) fn place(src: &Path, dir: &Path, name: &str) -> Result Result { + fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + let (stem, ext) = match name.rsplit_once('.') { + Some((s, e)) if !s.is_empty() => (s.to_string(), format!(".{e}")), + _ => (name.to_string(), String::new()), + }; + for i in 0..100_000u32 { + let d = if i == 0 { dir.join(name) } else { dir.join(format!("{stem}-{i}{ext}")) }; + match copy_verified(src, &d) { + Ok(()) => return Ok(d), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(format!("copy {}: {e} (nothing was imported from it; the original is untouched)", src.display())), + } + } + Err(format!("copy {}: no free name in {}", src.display(), dir.display())) +} + /// Undo a [`place`]: remove what it created at the destination (the sources were never touched). pub(crate) fn rollback(p: &Placed) { for sc in p.sidecars.iter().filter(|s| s.created) { diff --git a/crates/engine/src/tests_import_move.rs b/crates/engine/src/tests_import_move.rs index 59591065..5eb278fe 100644 --- a/crates/engine/src/tests_import_move.rs +++ b/crates/engine/src/tests_import_move.rs @@ -163,6 +163,50 @@ fn a_failed_or_corrupt_copy_keeps_the_source() { } } +/// Issue #96: Import → Copy used a plain unverified copy (and check-then-copy naming that could +/// replace a file). It now gets the same verified copy as Move: a copy that fails or comes out +/// different is removed and reported as a failed import, nothing is catalogued, and the card +/// is untouched; a good copy never replaces an existing file. +#[test] +fn copy_import_verifies_each_copy() { + let import_copy = |s: &mut Session, card: &Path, dest: &Path| { + s.execute( + "library.import", + &json!({"paths": [card.to_string_lossy()], "mode": "copy", "destination": dest.to_string_lossy(), "organize": "flat"}), + ) + .unwrap() + }; + for fault in [Fault::FailWrite, Fault::Corrupt] { + let base = temp_dir(&format!("copyfail-{fault:?}")); + let (card, dest) = (base.join("card"), base.join("out")); + write_png(&card.join("a.png"), 3); + let bytes = std::fs::read(card.join("a.png")).unwrap(); + let mut s = session(); + inject(fault); + let r = import_copy(&mut s, &card, &dest); + inject(Fault::None); + assert_eq!(len(&r, "failed"), 1, "{fault:?}: {r}"); + assert_eq!(len(&r, "imported"), 0, "{fault:?}: {r}"); + assert!(r.to_string().contains("the original is untouched"), "{r}"); + assert_eq!(s.catalog.len(), 0); + assert_eq!(std::fs::read(card.join("a.png")).unwrap(), bytes, "{fault:?}: source intact"); + assert_eq!(files_under(&dest), Vec::::new(), "{fault:?}: no bad copy left"); + let _ = std::fs::remove_dir_all(&base); + } + // a good copy: verified, and a taken name gets -1 instead of being replaced + let base = temp_dir("copyok"); + let (card, dest) = (base.join("card"), base.join("out")); + write_png(&card.join("a.png"), 4); + std::fs::create_dir_all(&dest).unwrap(); + std::fs::write(dest.join("a.png"), b"someone else's file").unwrap(); + let mut s = session(); + let r = import_copy(&mut s, &card, &dest); + assert_eq!(len(&r, "imported"), 1, "{r}"); + assert_eq!(std::fs::read(dest.join("a.png")).unwrap(), b"someone else's file"); + assert_eq!(std::fs::read(dest.join("a-1.png")).unwrap(), std::fs::read(card.join("a.png")).unwrap()); + let _ = std::fs::remove_dir_all(&base); +} + /// A source that can't be removed (a read-only card) stays and is reported; the photo is /// imported from its copy. #[test] diff --git a/docs/parity.md b/docs/parity.md index 2f24d599..a926a19e 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -116,7 +116,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-IMP-ADD-DIALOG | Add photos/folders | P0 | ✅ | `cmd:file.addPhotos`, `cmd:library.importPreview`, `cmd:library.import` (`mode`, `album`, `albumName`, `preset`, `keywords`), `crates/ui-egui/src/import.rs` | review dialog (Import Photos): the scanned source (a scanned folder is not added to Local), candidate grid with thumbnails and checkboxes (duplicates by path/content marked and unchecked), transfer mode (add in place / copy / move, each explained), album (existing/new), preset, keywords; the folder scan runs on a background thread with a progress window and Cancel (a NAS folder no longer freezes the window); batched import with a progress window; one undo step. Drag-and-drop still imports directly | | LR-IMP-DRAGDROP | Drop files/folders to import | P0 | ✅ | `crates/ui-egui/src/lib.rs` (dropped files → `cmd:library.import`) | dropping onto a specific album not supported | | LR-IMP-DUPES | Skip duplicates by content | P1 | ✅ | `crates/engine/src/import.rs`, `crates/engine/src/tests_import.rs` | | -| LR-IMP-DEVICE | Import from camera/card | P1 | ✅ | `cmd:library.devices`, `cmd:file.addFromDevice`, `crates/engine/src/devices.rs` | mounted volumes with a DCIM folder (macOS /Volumes, Linux /media and /run/media, Windows drive letters); File → Import from Device → the import review, copying into the library by default; menus serve the last scan and rescan in the background (never blocking a frame; hot-plugs repaint); no PTP/MTP (cameras that don't mount as a disk) | +| LR-IMP-DEVICE | Import from camera/card | P1 | ✅ | `cmd:library.devices`, `cmd:file.addFromDevice`, `crates/engine/src/devices.rs` | mounted volumes with a DCIM folder (macOS /Volumes, Linux /media and /run/media, Windows drive letters); File → Import from Device → the import review, copying into the library by default; menus serve the last scan and rescan in the background (never blocking a frame; hot-plugs repaint); copies are verified like Move's (a new file, synced, compared byte for byte; a bad copy is removed and reported as failed, never counted as imported; taken names get -1, -2…, never replaced — issue #96, `crates/engine/src/import_move.rs`); no PTP/MTP (cameras that don't mount as a disk) | | LR-IMP-AUTO | Watched-folder auto import | P2 | ✅ | `cmd:library.autoImport`, `cmd:library.autoImportScan`, `crates/ui-egui/src/panels/settings.rs` | Settings ▸ Import ▸ Auto Import: a watched folder whose new photos are added (in place or copied) once complete, into an optional album; scanned every 3 s | | LR-IMP-PRESET | Preset on import | P2 | ✅ | `cmd:library.import` (`preset`) | chosen in the import review; one History entry | | LR-IMP-RAWDEFAULT | Raw defaults | P1 | ✅ | `cmd:library.preferences`, `crates/engine/src/import.rs`, `crates/ui-egui/src/panels/settings.rs` | LightCraft default / a preset / per camera (make + model); non-raw default too; the preset look counts as unedited and Reset returns to it | diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..459a0607 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -10,6 +10,10 @@ - 23 new built-in presets: Portrait, Landscape, Urban, Food, Seasons, Vintage and B&W toners. ### Reliability +- Import ▸ Copy verifies every copy, like Move (issue #96): each file is written as a new file, synced to disk and + compared byte for byte with the card. A copy that fails or differs is removed and reported as a failed import — + so "import complete" means the copies are good before you format the card — and a name that is taken gets -1, + -2… instead of being replaced. - Canon CR2 photos from the EOS 7D, 50D, 60D, 550D, 600D, 1200D, 1300D, 5D Mark II and 1D Mark IV (and other models whose sensor starts on a green-blue row) no longer come out magenta (issue #85): the colour-filter layout is read from each file instead of assumed.