From ad2525ece291f299e5cbdf421f87cc6ff4bc963a Mon Sep 17 00:00:00 2001 From: benlukka <68825026+benlukka@users.noreply.github.com> Date: Tue, 6 Oct 2026 13:18:03 +0200 Subject: [PATCH 01/12] fix(ui): eliminate preview flickering when hovering presets and profiles - Retain active preset preview when hovering within the scroll or panel area across headers and gaps. - Prevent premature dropping of hover preview textures during asynchronous rerendering in detail view. - Clear cached hover state when clicking an item or when the pointer leaves the panel. - Add headless regression tests covering preset hover persistence and transitions. Signed-off-by: benlukka <68825026+benlukka@users.noreply.github.com> --- crates/ui-egui/src/headless.rs | 9 +++++++++ crates/ui-egui/src/panels/detail.rs | 4 ++-- crates/ui-egui/src/panels/presets.rs | 24 ++++++++++++++++++++++-- crates/ui-egui/src/panels/profiles.rs | 16 ++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index a8c6e062..52cb96f9 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -413,6 +413,15 @@ mod tests { assert_eq!(after.develop, before.develop); assert_eq!(after.history.len(), before.history.len(), "no history entry while hovering"); assert!(h.app.session.undo.is_empty()); + // hovering down across group headers within the presets list retains the preview (no flicker) + h.request("ui.hoverWidget", json!({"id": "presetGroup:Creative"}), t); + h.step(); + assert_eq!(h.app.hover_preview.as_ref().map(|p| p.label.as_str()), Some("Preset: High Contrast B&W")); + // hovering another preset switches smoothly to it + h.request("ui.hoverWidget", json!({"id": "preset:lc.warm-glow"}), t); + h.settle(SETTLE); + h.step(); + assert_eq!(h.app.hover_preview.as_ref().map(|p| p.label.as_str()), Some("Preset: Warm Glow")); // thumbnails: variant textures for the visible presets h.app.ui.preset_thumbs = true; h.request("ui.move", json!({"x": 5, "y": 500}), t); diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index a8fbc638..0697b75c 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -280,8 +280,8 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { } else if show_before { shown = draw(Slot::Before, img_rect); p.text(pos2(img_rect.left() + 8.0, img_rect.top() + 14.0), Align2::LEFT_CENTER, crate::i18n::tr("Before"), t.font(12.0), t.text); - } else if let Some((key, label)) = &hover_key - && let Some(tex) = app.renderer.textures.get(&Slot::Hover).filter(|t| t.photo == id && t.key == *key) + } else if let Some((_key, label)) = &hover_key + && let Some(tex) = app.renderer.textures.get(&Slot::Hover).filter(|t| t.photo == id) { p.image(tex.tex.id(), img_rect, Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 1.0)), Color32::WHITE); let g = p.layout_no_wrap(label.clone(), t.font(12.0), Color32::WHITE); diff --git a/crates/ui-egui/src/panels/presets.rs b/crates/ui-egui/src/panels/presets.rs index 74b1e129..20a095e0 100644 --- a/crates/ui-egui/src/panels/presets.rs +++ b/crates/ui-egui/src/panels/presets.rs @@ -78,7 +78,8 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let current = active.and_then(|id| app.session.develop_of(id)).map(|d| (*d).clone()); let thumbs = app.ui.preset_thumbs && active.is_some(); let row_h = if thumbs { 54.0 } else { 26.0 }; - egui::ScrollArea::vertical().id_salt("presets-scroll").auto_shrink([false, false]).show(ui, |ui| { + let scroll = egui::ScrollArea::vertical().id_salt("presets-scroll").auto_shrink([false, false]).show(ui, |ui| { + ui.spacing_mut().item_spacing.y = 0.0; let mut groups: BTreeMap> = BTreeMap::new(); for p in &app.session.presets { groups.entry(p.group.clone()).or_default().push(p.clone()); @@ -141,11 +142,16 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { if resp.hovered() && let Some(s) = look { - app.hover_preview = Some(HoverPreview { label: crate::i18n::tr_format!("Preset: {name}", name = name), settings: s }); + let label = crate::i18n::tr_format!("Preset: {name}", name = name); + app.hover_preview = Some(HoverPreview { label: label.clone(), settings: s }); + if let Some(id) = active { + ui.data_mut(|d| d.insert_temp(egui::Id::new("last-preset-hover"), (id, pr.clone(), label))); + } } if resp.clicked() { let _ = app.run("preset.apply", json!({"id": pid, "amount": 100})); ui.data_mut(|d| d.insert_temp(amt_id, (pid.clone(), 100.0))); + ui.data_mut(|d| d.remove::<(lightcraft_catalog::PhotoId, Preset, String)>(egui::Id::new("last-preset-hover"))); app.toast(ui.ctx(), crate::i18n::tr_format!("Preset: {name}", name = name)); } let (builtin, group) = (pr.builtin, pr.group.clone()); @@ -207,6 +213,20 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { } ui.add_space(30.0); }); + let last_hover_id = egui::Id::new("last-preset-hover"); + let pointer_in_scroll = ui.ctx().input(|i| i.pointer.hover_pos()).is_some_and(|pos| scroll.inner_rect.contains(pos)); + if pointer_in_scroll { + if app.hover_preview.is_none() + && let (Some(id), Some(curr)) = (active, current.as_ref()) + && let Some((saved_id, saved_preset, saved_label)) = + ui.data(|d| d.get_temp::<(lightcraft_catalog::PhotoId, Preset, String)>(last_hover_id)) + && saved_id == id + { + app.hover_preview = Some(HoverPreview { label: saved_label, settings: saved_preset.apply(curr, 1.0) }); + } + } else { + ui.data_mut(|d| d.remove::<(lightcraft_catalog::PhotoId, Preset, String)>(last_hover_id)); + } }); } diff --git a/crates/ui-egui/src/panels/profiles.rs b/crates/ui-egui/src/panels/profiles.rs index ff54f446..7633cdef 100644 --- a/crates/ui-egui/src/panels/profiles.rs +++ b/crates/ui-egui/src/panels/profiles.rs @@ -81,6 +81,20 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) { ui.add_space(6.0); } ui.add_space(30.0); + let last_hover_id = egui::Id::new("last-profile-hover"); + let pointer_in_profiles = ui.ctx().input(|i| i.pointer.hover_pos()).is_some_and(|pos| ui.max_rect().contains(pos)); + if pointer_in_profiles { + if app.hover_preview.is_none() + && let Some((saved_id, saved_profile_id)) = ui.data(|m| m.get_temp::<(PhotoId, String)>(last_hover_id)) + && saved_id == id + && let Some(p) = profile(&saved_profile_id) + { + let s = with_profile(&d, p); + app.hover_preview = Some(HoverPreview { label: crate::i18n::tr_format!("Profile: {}", p.name), settings: s }); + } + } else { + ui.data_mut(|m| m.remove::<(PhotoId, String)>(last_hover_id)); + } } /// `d` with profile `p` (at 100 % unless it is the applied one). @@ -127,6 +141,7 @@ fn cell_ui(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &DevelopSettings, id: } if resp.hovered() && !applied && !over_star { app.hover_preview = Some(HoverPreview { label: crate::i18n::tr_format!("Profile: {}", p.name), settings: s.clone() }); + ui.data_mut(|m| m.insert_temp(egui::Id::new("last-profile-hover"), (id, p.id.to_string()))); } let resp = resp.on_hover_text(format!("{} ({})", p.name, p.group)); if resp.clicked() { @@ -134,6 +149,7 @@ fn cell_ui(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &DevelopSettings, id: let _ = app.run("profile.favorite", json!({"id": p.id})); } else if !applied { let _ = app.run("develop.profile", json!({"id": p.id, "amount": 100})); + ui.data_mut(|m| m.remove::<(PhotoId, String)>(egui::Id::new("last-profile-hover"))); } } } From 11ebb3bdb6c4af0d940ff010b63fa45bd5e21e41 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 07:50:38 -0400 Subject: [PATCH 02/12] Windows packaging: Start Menu/Add-Remove icon, no console window, correct description The MSI Icon id had no .ico/.exe extension, which Windows Installer needs for advertised shortcuts and ARPPRODUCTICON, so both showed a blank icon. Release builds of the desktop app are now GUI-subsystem (no console window; the CLI stays a console program), the shortcut/summary description names what LightCraft does, and packaging-lint rejects an Icon id without an extension. Found in printcraft#57. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/packaging-lint.yml | 4 ++++ packaging/windows/lightcraft.wxs | 10 +++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/packaging-lint.yml b/.github/workflows/packaging-lint.yml index 41b361cb..2b53ae6c 100644 --- a/.github/workflows/packaging-lint.yml +++ b/.github/workflows/packaging-lint.yml @@ -47,6 +47,10 @@ jobs: - name: XML, desktop entry, AppStream, Flatpak manifest run: | xmllint --noout packaging/windows/lightcraft.wxs packaging/macos/*.plist* packaging/linux/*.xml* + # Windows Installer needs Icon ids ending in .ico or .exe, or shortcuts show a blank icon. + if grep -oE '"$RUNNER_TEMP/ai.storyteller.lightcraft.metainfo.xml" appstreamcli validate --no-net --explain "$RUNNER_TEMP/ai.storyteller.lightcraft.metainfo.xml" diff --git a/packaging/windows/lightcraft.wxs b/packaging/windows/lightcraft.wxs index 9d41899d..37510455 100644 --- a/packaging/windows/lightcraft.wxs +++ b/packaging/windows/lightcraft.wxs @@ -17,15 +17,15 @@ Scope="perMachine" Compressed="yes" Language="1033"> - + - - + + @@ -45,8 +45,8 @@ + Description="LightCraft photo library and raw developer" WorkingDirectory="INSTALLFOLDER" Advertise="yes" + Icon="LightcraftIcon.ico" IconIndex="0" /> From fec3b1523493f350e837ad20eed9c86606eac8ac Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 07:53:44 -0400 Subject: [PATCH 03/12] release.yml: explicit if for the wasm-bindgen install (shellcheck SC2015) packaging-lint has failed on main since this step was added; same behaviour. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/release.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7b04f35f..8b3bdea3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -221,8 +221,9 @@ jobs: - name: Install wasm-bindgen CLI run: | # Must match the wasm-bindgen version in Cargo.lock exactly (cargo xtask web checks it). - command -v wasm-bindgen >/dev/null && wasm-bindgen --version | grep -q "$WASM_BINDGEN_VERSION" \ - || cargo install wasm-bindgen-cli --version "$WASM_BINDGEN_VERSION" --locked + if ! { command -v wasm-bindgen >/dev/null && wasm-bindgen --version | grep -q "$WASM_BINDGEN_VERSION"; }; then + cargo install wasm-bindgen-cli --version "$WASM_BINDGEN_VERSION" --locked + fi wasm-bindgen --version - name: Apply version override if: inputs.version != '' From d0612b0b21899c076e8d8af11e9294cbbca7a3e5 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 08:01:29 -0400 Subject: [PATCH 04/12] Release: LightCraft v0.2.1 Fixes the Windows installer: the Start Menu shortcut and Add/Remove Programs icon were blank, and the shortcut description was wrong. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 44 ++++++++++++++++++++++---------------------- Cargo.toml | 2 +- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 07324919..1952f5c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2073,7 +2073,7 @@ dependencies = [ [[package]] name = "lightcraft" -version = "0.2.0" +version = "0.2.1" dependencies = [ "eframe", "egui", @@ -2091,7 +2091,7 @@ dependencies = [ [[package]] name = "lightcraft-catalog" -version = "0.2.0" +version = "0.2.1" dependencies = [ "crc32fast", "lightcraft-develop", @@ -2105,7 +2105,7 @@ dependencies = [ [[package]] name = "lightcraft-cli" -version = "0.2.0" +version = "0.2.1" dependencies = [ "dhat", "lightcraft-codecs", @@ -2120,7 +2120,7 @@ dependencies = [ [[package]] name = "lightcraft-codecs" -version = "0.2.0" +version = "0.2.1" dependencies = [ "bytemuck", "image", @@ -2145,7 +2145,7 @@ dependencies = [ [[package]] name = "lightcraft-color" -version = "0.2.0" +version = "0.2.1" dependencies = [ "lightcraft-geom", "proptest", @@ -2154,7 +2154,7 @@ dependencies = [ [[package]] name = "lightcraft-develop" -version = "0.2.0" +version = "0.2.1" dependencies = [ "lightcraft-color", "lightcraft-geom", @@ -2165,7 +2165,7 @@ dependencies = [ [[package]] name = "lightcraft-engine" -version = "0.2.0" +version = "0.2.1" dependencies = [ "ab_glyph", "libc", @@ -2193,7 +2193,7 @@ dependencies = [ [[package]] name = "lightcraft-geom" -version = "0.2.0" +version = "0.2.1" dependencies = [ "proptest", "serde", @@ -2201,7 +2201,7 @@ dependencies = [ [[package]] name = "lightcraft-gpu" -version = "0.2.0" +version = "0.2.1" dependencies = [ "bytemuck", "lightcraft-color", @@ -2217,7 +2217,7 @@ dependencies = [ [[package]] name = "lightcraft-mcp" -version = "0.2.0" +version = "0.2.1" dependencies = [ "lightcraft-codecs", "lightcraft-engine", @@ -2228,7 +2228,7 @@ dependencies = [ [[package]] name = "lightcraft-merge" -version = "0.2.0" +version = "0.2.1" dependencies = [ "lightcraft-codecs", "lightcraft-color", @@ -2244,7 +2244,7 @@ dependencies = [ [[package]] name = "lightcraft-meta" -version = "0.2.0" +version = "0.2.1" dependencies = [ "lightcraft-geom", "lightcraft-tiff", @@ -2256,7 +2256,7 @@ dependencies = [ [[package]] name = "lightcraft-pipeline" -version = "0.2.0" +version = "0.2.1" dependencies = [ "image", "lightcraft-color", @@ -2272,7 +2272,7 @@ dependencies = [ [[package]] name = "lightcraft-preview" -version = "0.2.0" +version = "0.2.1" dependencies = [ "jpeg-encoder", "lightcraft-raster", @@ -2284,7 +2284,7 @@ dependencies = [ [[package]] name = "lightcraft-raster" -version = "0.2.0" +version = "0.2.1" dependencies = [ "libc", "lightcraft-color", @@ -2296,7 +2296,7 @@ dependencies = [ [[package]] name = "lightcraft-raw" -version = "0.2.0" +version = "0.2.1" dependencies = [ "jpeg-encoder", "lightcraft-color", @@ -2315,7 +2315,7 @@ dependencies = [ [[package]] name = "lightcraft-scenes" -version = "0.2.0" +version = "0.2.1" dependencies = [ "image", "lightcraft-color", @@ -2326,11 +2326,11 @@ dependencies = [ [[package]] name = "lightcraft-sysmem" -version = "0.2.0" +version = "0.2.1" [[package]] name = "lightcraft-tiff" -version = "0.2.0" +version = "0.2.1" dependencies = [ "proptest", "serde", @@ -2339,7 +2339,7 @@ dependencies = [ [[package]] name = "lightcraft-ui-egui" -version = "0.2.0" +version = "0.2.1" dependencies = [ "egui", "lightcraft-catalog", @@ -2359,7 +2359,7 @@ dependencies = [ [[package]] name = "lightcraft-web" -version = "0.2.0" +version = "0.2.1" dependencies = [ "crc32fast", "eframe", @@ -5128,7 +5128,7 @@ checksum = "e450f9b2ed1dff33c94c12589a87338689467b9c4f5d8a5710bd09a847d2c8a7" [[package]] name = "xtask" -version = "0.2.0" +version = "0.2.1" dependencies = [ "brotli", "flate2", diff --git a/Cargo.toml b/Cargo.toml index 7a2ee8a5..b1882f9b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "3" members = ["crates/*", "apps/*", "xtask"] [workspace.package] -version = "0.2.0" +version = "0.2.1" edition = "2024" license = "MIT OR Apache-2.0" rust-version = "1.90" From f5bd3cc2dbe83247a21dd5d433f4c6cbead96dab Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 09:38:47 -0400 Subject: [PATCH 05/12] Windows on ARM64: release builds, PE header check, install test on ARM64 hardware Release builds add an arm64 MSI and portable zip, cross-compiled on the x64 runner (signing and WiX as for x64). package.ps1 checks each binary's PE header before packaging: Machine must match -Arch, the app must be a GUI program and the CLI a console program. It skips the CLI smoke run when an ARM64 build can't run on the host. A new Windows ARM64 workflow packages the ARM64 MSI, installs it on a Windows 11 ARM64 runner, checks both installed exes are ARM64, runs the CLI, and uninstalls. Ported from printcraft#66. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/release.yml | 3 + .github/workflows/windows-arm64.yml | 95 +++++++++++++++++++++++++++++ packaging/windows/package.ps1 | 31 +++++++++- 3 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/windows-arm64.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8b3bdea3..97cf75d5 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -112,6 +112,9 @@ jobs: include: - { arch: x64, target: x86_64-pc-windows-msvc } - { arch: x86, target: i686-pc-windows-msvc } + # Windows on ARM: cross-compiled on the x64 runner, so signing and WiX run as above. + # .github/workflows/windows-arm64.yml installs and runs it on ARM64 hardware. + - { arch: arm64, target: aarch64-pc-windows-msvc } env: LIGHTCRAFT_VERSION: ${{ needs.version.outputs.version }} LIGHTCRAFT_BUILD_DATE: ${{ needs.version.outputs.date }} diff --git a/.github/workflows/windows-arm64.yml b/.github/workflows/windows-arm64.yml new file mode 100644 index 00000000..409c8826 --- /dev/null +++ b/.github/workflows/windows-arm64.yml @@ -0,0 +1,95 @@ +# Windows on ARM: package the ARM64 release the way release.yml does (cross-compiled on an x64 +# runner, unsigned here), then install that MSI on a Windows 11 ARM64 runner, check the installed +# binaries are ARM64, run the CLI natively, and uninstall. (A native ARM64 `cargo test` job is left +# out for now: PrintCraft saw an intermittent access violation in egui UI tests there.) +name: Windows ARM64 + +on: + push: + branches: [main] + pull_request: + paths: + - Cargo.toml + - Cargo.lock + - "**/Cargo.toml" + - "**/build.rs" + - .github/workflows/windows-arm64.yml + - packaging/windows/** + schedule: + - cron: "41 4 * * 1" # weekly, so drift in the runner images shows up + workflow_dispatch: + +concurrency: + group: windows-arm64-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +env: + CARGO_TERM_COLOR: always + CARGO_INCREMENTAL: 0 + +jobs: + package: + name: package (cross-compiled on x64) + runs-on: windows-latest + timeout-minutes: 90 + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + targets: aarch64-pc-windows-msvc + - uses: Swatinem/rust-cache@v2 + with: + key: windows-arm64-package + - name: Install WiX + shell: pwsh + run: dotnet tool install --global wix --version 5.0.2 + - name: Build and package (unsigned) + shell: pwsh + env: + DIST: ${{ github.workspace }}\dist\arm64 + run: ./packaging/windows/package.ps1 -Arch arm64 + - uses: actions/upload-artifact@v4 + with: + name: lightcraft-windows-arm64 + path: dist/arm64/* + if-no-files-found: error + retention-days: 7 + + install: + name: install and run on ARM64 + needs: package + runs-on: windows-11-arm + timeout-minutes: 30 + steps: + - uses: actions/download-artifact@v4 + with: + name: lightcraft-windows-arm64 + path: dist + - name: Install the MSI, check it, run it, uninstall it + shell: pwsh + run: | + $ErrorActionPreference = 'Stop' + $msi = (Get-ChildItem dist/*.msi | Select-Object -First 1).FullName + $p = Start-Process msiexec.exe -ArgumentList '/i', "`"$msi`"", '/qn', '/l*v', 'install.log' -Wait -PassThru + if ($p.ExitCode -ne 0) { Get-Content install.log -Tail 60; throw "msiexec /i exited $($p.ExitCode)" } + $dir = Join-Path $env:ProgramFiles 'LightCraft' + foreach ($exe in 'lightcraft.exe', 'lightcraft-cli.exe') { + $path = Join-Path $dir $exe + if (-not (Test-Path $path)) { throw "$exe not installed in $dir" } + $bytes = [System.IO.File]::ReadAllBytes($path) + $machine = [BitConverter]::ToUInt16($bytes, [BitConverter]::ToInt32($bytes, 0x3C) + 4) + if ($machine -ne 0xAA64) { throw "$exe is for machine 0x$('{0:X}' -f $machine), not ARM64" } + Write-Output "ok $path (ARM64)" + } + # The installed CLI runs natively on ARM64. + $cli = Join-Path $dir 'lightcraft-cli.exe' + $out = & $cli --version 2>&1 | Out-String + if ($LASTEXITCODE -ne 0) { Write-Output $out; throw "lightcraft-cli --version exited $LASTEXITCODE" } + Write-Output "ok lightcraft-cli --version: $($out.Trim())" + $p = Start-Process msiexec.exe -ArgumentList '/x', "`"$msi`"", '/qn' -Wait -PassThru + if ($p.ExitCode -ne 0) { throw "msiexec /x exited $($p.ExitCode)" } + if (Test-Path (Join-Path $dir 'lightcraft.exe')) { throw 'uninstall left lightcraft.exe behind' } + Write-Output 'ok uninstalled' diff --git a/packaging/windows/package.ps1 b/packaging/windows/package.ps1 index ba6ff63d..45ced0e5 100644 --- a/packaging/windows/package.ps1 +++ b/packaging/windows/package.ps1 @@ -17,9 +17,10 @@ .EXAMPLE pwsh packaging/windows/package.ps1 -Arch x64 pwsh packaging/windows/package.ps1 -Arch x86 -SkipBuild + pwsh packaging/windows/package.ps1 -Arch arm64 # cross-compiled; needs the MSVC ARM64 build tools #> param( - [ValidateSet('x64', 'x86')] [string] $Arch = 'x64', + [ValidateSet('x64', 'x86', 'arm64')] [string] $Arch = 'x64', [switch] $SkipBuild ) $ErrorActionPreference = 'Stop' @@ -44,7 +45,7 @@ if (-not $Version) { throw 'could not read [workspace.package] version from Carg # MSI ProductVersion is numeric (major.minor.build); pre-release tags are dropped there. $MsiVersion = ($Version -split '-')[0] -$Target = if ($Arch -eq 'x64') { 'x86_64-pc-windows-msvc' } else { 'i686-pc-windows-msvc' } +$Target = switch ($Arch) { 'x64' { 'x86_64-pc-windows-msvc' } 'x86' { 'i686-pc-windows-msvc' } 'arm64' { 'aarch64-pc-windows-msvc' } } $Dist = if ($env:DIST) { $env:DIST } else { Join-Path $Root 'dist\release' } $TargetDir = if ($env:CARGO_TARGET_DIR) { $env:CARGO_TARGET_DIR } else { Join-Path $Root 'target' } New-Item -ItemType Directory -Force -Path $Dist | Out-Null @@ -65,6 +66,23 @@ if (-not $SkipBuild) { } $Bin = Join-Path $TargetDir "$Target\release" + +# Check both binaries' PE headers before packaging. Machine (COFF header) must match -Arch, so an +# x64 build can never ship labelled arm64. Subsystem (optional header): 2 = Windows GUI, 3 = console. +# The app must be GUI (no console window opens with it); the CLI must stay console so its output +# reaches the terminal. +function Get-PeHeader([string] $Path) { + $bytes = [System.IO.File]::ReadAllBytes($Path) + $pe = [BitConverter]::ToInt32($bytes, 0x3C) + return @{ Machine = [BitConverter]::ToUInt16($bytes, $pe + 4); Subsystem = [BitConverter]::ToUInt16($bytes, $pe + 0x5C) } +} +$Machine = switch ($Arch) { 'x64' { 0x8664 } 'x86' { 0x14C } 'arm64' { 0xAA64 } } +foreach ($check in @(@('lightcraft.exe', 2), @('lightcraft-cli.exe', 3))) { + $h = Get-PeHeader (Join-Path $Bin $check[0]) + if ($h.Machine -ne $Machine) { throw "$($check[0]) is for machine 0x$('{0:X}' -f $h.Machine), expected 0x$('{0:X}' -f $Machine) ($Arch)" } + if ($h.Subsystem -ne $check[1]) { throw "$($check[0]) has PE subsystem $($h.Subsystem), expected $($check[1])" } + Write-Output "ok $($check[0]): $Arch, PE subsystem $($h.Subsystem)" +} $Stage = Join-Path $TargetDir "windows-package\$Arch" Remove-Item -Recurse -Force $Stage -ErrorAction SilentlyContinue New-Item -ItemType Directory -Force -Path $Stage | Out-Null @@ -96,5 +114,12 @@ $Zip = Join-Path $Dist "lightcraft-$Version-windows-$Arch-portable.zip" Remove-Item -Force $Zip -ErrorAction SilentlyContinue Compress-Archive -Path $Portable -DestinationPath $Zip -Invoke-Native 'lightcraft-cli --version' { & (Join-Path $Stage 'lightcraft-cli.exe') --version } +# Smoke-test the CLI when this machine can run it. An ARM64 build made on an x64 runner can't run +# here; .github/workflows/windows-arm64.yml installs and runs it on ARM64 instead. +$HostArch = [System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture.ToString().ToLowerInvariant() +if ($Arch -ne 'arm64' -or $HostArch -eq 'arm64') { + Invoke-Native 'lightcraft-cli --version' { & (Join-Path $Stage 'lightcraft-cli.exe') --version } +} else { + Write-Output "skipping lightcraft-cli --version: an $Arch build doesn't run on this $HostArch machine" +} Get-Item $Msi, $Zip | Format-Table Name, Length From a2dbefd1ee09aa02f8ad530de871314bd5f964ab Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 09:51:08 -0400 Subject: [PATCH 06/12] Windows ARM64 workflow: pin third-party actions by commit Co-Authored-By: Claude Opus 5.5 --- .github/workflows/windows-arm64.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/windows-arm64.yml b/.github/workflows/windows-arm64.yml index 409c8826..f9f8cef2 100644 --- a/.github/workflows/windows-arm64.yml +++ b/.github/workflows/windows-arm64.yml @@ -37,10 +37,10 @@ jobs: timeout-minutes: 90 steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@89b12181fb390509a0842a86cc55eeb8eb928c1d # stable with: targets: aarch64-pc-windows-msvc - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2 with: key: windows-arm64-package - name: Install WiX From 7a2596cd4369a7e80e56184d98c2219c878a4a4f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 6 Oct 2026 10:08:29 -0400 Subject: [PATCH 07/12] Issue #134: Exports: atomic without fsync; Copy import verifies against the probe hash - safe_file: write policy documented (durable for what can't be recreated, atomic-only for recreatable outputs); new write_atomic_nosync (temp + rename, temp removed on failure, no file/folder sync); per-thread sync counter for tests. - export::write_file (exports, renders, screenshots, merge previews, CLI/MCP) uses the non-syncing writer; new write_file_durable keeps the sync for Edit a Copy (the TIFF becomes a library photo). Catalog, sidecars, settings, DNG conversion/merges and smart previews stay durable; the #93 original-overwrite guard is untouched. - Import > Copy verifies the synced copy against the probe's content hash (SipHash-1-3/128 of the whole file) instead of re-reading the source; byte compare when there is no hash. A file changed since Review Import is now refused instead of imported with stale details. Move keeps its byte-for-byte check before deleting sources. - check_write_target: skip building the library-wide guard when nothing is at the target. - write_new_unique keeps its read-back (DNGs replace raws; cheap next to encoding). - Tests: no sync on the export path, durable writers still sync, Copy caught by the probe hash (corrupt copy, stale hash, changed source); ignored timing test write_policy_timing. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/safe_file.rs | 87 +++++++++++++++++-- crates/engine/src/cmd/convert.rs | 3 +- crates/engine/src/cmd/previews.rs | 4 +- crates/engine/src/export.rs | 20 ++++- crates/engine/src/import.rs | 11 ++- crates/engine/src/import_move.rs | 48 ++++++++--- crates/engine/src/media.rs | 4 +- crates/engine/src/originals.rs | 9 +- crates/engine/src/tests_export.rs | 21 +++++ crates/engine/src/tests_import_move.rs | 113 +++++++++++++++++++++++++ docs/parity.md | 4 +- docs/whats-new.md | 19 +++-- 12 files changed, 303 insertions(+), 40 deletions(-) diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index 1e482b81..df239a17 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -1,11 +1,23 @@ //! Crash-safe writes of user-visible files: exports, renders, DNG conversions, smart previews and //! XMP sidecars all go through here (the catalog's own files use [`crate::store::FsStore`]). //! +//! **Write policy** (issue #134): what can't be recreated — originals, XMP sidecars, the catalog, +//! settings, DNGs that replace raws, smart previews (the only stand-in while an original is +//! offline), an edit copy that becomes a library photo — is written **durably**: synced to disk +//! before it counts as written. What can always be made again from the originals — exports, +//! renders, screenshots, merge previews — is written **atomically** only: a half-written file +//! never replaces a good one, but there is no per-file sync (on a USB drive or a NAS that sync +//! dominates a large export). +//! //! - [`write_atomic`] replaces a file so that after a crash, a full disk or an unplugged drive //! either the old or the complete new content is there — never a truncated mix. The content //! goes to a new temp file in the same folder (unique name, `create_new`), is synced to disk and //! renamed over the target; then the folder is synced (POSIX). Any failure removes the temp //! file and leaves the target as it was. +//! - [`write_atomic_nosync`] is the same temp file + rename without the two syncs, for +//! recreatable outputs only: a failed write still removes the temp file and leaves the target as +//! it was, but after a power cut the new file may be lost (on some file systems left empty) — +//! it is then simply exported again. //! - [`write_new`] / [`write_new_unique`] write a file that must not exist yet: the same temp file //! and sync, plus a read-back check, and the file then appears under its final name **without //! replacing anything** (a hard link, which fails on a taken name; a check-then-rename where the @@ -21,6 +33,19 @@ use std::sync::atomic::{AtomicU64, Ordering}; thread_local! { static FAIL_AFTER: Cell> = const { Cell::new(None) }; + static SYNCS: Cell = const { Cell::new(0) }; +} + +/// How many file and folder syncs this module made **on the calling thread** (for tests: a +/// durable write path syncs, a recreatable output doesn't). +#[doc(hidden)] +pub fn syncs_on_this_thread() -> u64 { + SYNCS.with(Cell::get) +} + +fn sync_file(f: &File) -> io::Result<()> { + SYNCS.with(|c| c.set(c.get() + 1)); + f.sync_all() } /// Fault injection for tests (in any crate): while the returned guard lives, every write made by @@ -78,7 +103,7 @@ fn parent_of(path: &Path) -> &Path { /// where NTFS journals the rename itself, so errors are ignored. pub fn sync_dir(dir: &Path) { if let Ok(d) = File::open(dir) { - let _ = d.sync_all(); + let _ = sync_file(&d); } } @@ -100,9 +125,9 @@ fn create_temp(path: &Path) -> io::Result<(PathBuf, File)> { Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("no free temp file name in {}", dir.display()))) } -/// Write a temp file next to `path` with what `fill` writes, synced to disk. On failure the temp -/// file is removed. Returns its path and length. -fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> { +/// Write a temp file next to `path` with what `fill` writes, synced to disk when `sync`. On +/// failure the temp file is removed. Returns its path and length. +fn write_temp(path: &Path, sync: bool, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> { let (tmp, file) = create_temp(path)?; let r = (|| { let limit = FAIL_AFTER.with(Cell::get); @@ -111,7 +136,9 @@ fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<() w.flush()?; let n = w.n; let f = w.inner.into_inner().map_err(|e| e.into_error())?; - f.sync_all()?; + if sync { + sync_file(&f)?; + } Ok(n) })(); match r { @@ -132,12 +159,26 @@ pub fn write_atomic(path: &Path, data: &[u8]) -> io::Result<()> { /// [`write_atomic`] with streamed content: `fill` writes it (an error aborts and leaves the old /// file). Returns the length written. pub fn write_atomic_with(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result { - let (tmp, n) = write_temp(path, fill)?; + replace_with(path, true, fill) +} + +/// Replace (or create) `path` with `data` atomically but **without syncing** it to disk — only for +/// outputs that can always be recreated (exports, renders, screenshots); see the module docs. +/// Anything that can't be made again goes through [`write_atomic`]. +pub fn write_atomic_nosync(path: &Path, data: &[u8]) -> io::Result<()> { + replace_with(path, false, &mut |w| w.write_all(data)).map(|_| ()) +} + +/// Temp file (synced when `sync`) renamed over `path`, then the folder synced when `sync`. +fn replace_with(path: &Path, sync: bool, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result { + let (tmp, n) = write_temp(path, sync, fill)?; if let Err(e) = fs::rename(&tmp, path) { let _ = fs::remove_file(&tmp); return Err(e); } - sync_dir(parent_of(path)); + if sync { + sync_dir(parent_of(path)); + } Ok(n) } @@ -155,8 +196,11 @@ pub fn write_new_unique(candidates: &mut dyn Iterator, data: &[u let Some(first) = candidates.peek().cloned() else { return Err(io::Error::new(io::ErrorKind::InvalidInput, "no file name to write")); }; - let (tmp, _) = write_temp(&first, &mut |w| w.write_all(data))?; + let (tmp, _) = write_temp(&first, true, &mut |w| w.write_all(data))?; let r = (|| { + // Kept (issue #134 point 4): these files replace raws (Convert / Copy as DNG) or become + // library photos (merges). On a local disk the read usually comes from the cache, but on a + // network volume it can go back to the server, and next to encoding a DNG it costs little. if !same_content(&tmp, data)? { return Err(io::Error::other("the written file reads back different (failing drive or connection?)")); } @@ -287,6 +331,33 @@ mod tests { let _ = fs::remove_dir_all(&d); } + #[test] + fn recreatable_outputs_are_atomic_without_a_sync() { + let d = temp("nosync"); + let p = d.join("export.jpg"); + let before = syncs_on_this_thread(); + write_atomic_nosync(&p, b"one").unwrap(); + write_atomic_nosync(&p, b"two two").unwrap(); + assert_eq!(syncs_on_this_thread(), before, "no file or folder sync"); + assert_eq!(fs::read(&p).unwrap(), b"two two"); + assert_eq!(names(&d), vec!["export.jpg"], "no temp file left"); + // still atomic: a failure part-way leaves the previous file and no temp file + { + let _f = fail_writes_after(3); + assert!(write_atomic_nosync(&p, &[9u8; 10_000]).is_err()); + } + assert_eq!(fs::read(&p).unwrap(), b"two two"); + assert_eq!(names(&d), vec!["export.jpg"]); + // the durable writers do sync (the file; the folder too where it can be opened) + let before = syncs_on_this_thread(); + write_atomic(&p, b"three").unwrap(); + let mid = syncs_on_this_thread(); + assert!(mid > before); + write_new(&d.join("new.dng"), b"dng").unwrap(); + assert!(syncs_on_this_thread() > mid); + let _ = fs::remove_dir_all(&d); + } + #[test] fn new_files_never_replace_anything() { let d = temp("new"); diff --git a/crates/engine/src/cmd/convert.rs b/crates/engine/src/cmd/convert.rs index c81267bf..1641d63f 100644 --- a/crates/engine/src/cmd/convert.rs +++ b/crates/engine/src/cmd/convert.rs @@ -149,7 +149,8 @@ fn edit_external(s: &mut Session, p: &Value) -> Result { }; let mut written = Vec::new(); let mut write = |path: &str, bytes: &[u8]| -> std::result::Result<(), String> { - crate::export::write_file(path, bytes)?; + // durable: the TIFF becomes a library photo the external editor then changes + crate::export::write_file_durable(path, bytes)?; written.push(path.to_string()); Ok(()) }; diff --git a/crates/engine/src/cmd/previews.rs b/crates/engine/src/cmd/previews.rs index d3b63389..3dacc31f 100644 --- a/crates/engine/src/cmd/previews.rs +++ b/crates/engine/src/cmd/previews.rs @@ -160,7 +160,9 @@ fn smart_run( if damaged && crate::smart::is_valid(&path) { n.built += 1; } else { - // atomic: a failed write leaves no partial proxy that would pass for a built one + // atomic: a failed write leaves no partial proxy that would pass for a built one; + // and synced (issue #134): built so the photo can be edited while its original + // is offline, when it is the only copy — the sync is small next to the decode let r = source .ok_or_else(|| "nothing to build from".to_string()) .and_then(|s| s.load_source()) diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index 6c30b1a0..d40ab577 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -1131,15 +1131,27 @@ pub fn export_batch( } /// Write an exported or rendered file on disk: its folder is created if needed, and the file is -/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic`]: a temp file, synced, then -/// renamed), so a failure part-way leaves any previous file intact and no truncated one. The -/// native writer behind exports, renders and screenshots (app, CLI, MCP). +/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic_nosync`]: a temp file +/// renamed into place), so a failure part-way leaves any previous file intact and no truncated +/// one. Not synced to disk (issue #134): an export can always be made again from the original. +/// The native writer behind exports, renders and screenshots (app, CLI, MCP). pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> { + write_with(path, bytes, false) +} + +/// [`write_file`], synced to disk: for a written file that becomes a library photo (Edit a Copy) +/// and so can't simply be exported again once the catalog points at it. +pub fn write_file_durable(path: &str, bytes: &[u8]) -> Result<(), String> { + write_with(path, bytes, true) +} + +fn write_with(path: &str, bytes: &[u8], durable: bool) -> Result<(), String> { + use lightcraft_catalog::safe_file::{write_atomic, write_atomic_nosync}; let p = std::path::Path::new(path); if let Some(dir) = p.parent().filter(|d| !d.as_os_str().is_empty()) { std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; } - lightcraft_catalog::safe_file::write_atomic(p, bytes).map_err(|e| format!("{path}: {e}")) + if durable { write_atomic(p, bytes) } else { write_atomic_nosync(p, bytes) }.map_err(|e| format!("{path}: {e}")) } /// The path of the sidecar with extension `ext` of the exported file `main`. diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index a85d6287..6f35b670 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -375,13 +375,16 @@ 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. 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 { +/// synced and checked against `probe_hash`, the content hash the probe computed — or compared +/// byte for byte with the source when there is none); a bad one is removed and the photo +/// reported as failed. +fn copy_into(root: &Path, src: &str, folders: &[String], name: Option<&str>, probe_hash: Option<&str>) -> Result { let dir = folders.iter().fold(root.to_path_buf(), |d, f| d.join(f)); 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())); - crate::import_move::copy_new(Path::new(src), &dir, &name).map(|p| p.to_string_lossy().to_string()) + let expect = probe_hash.and_then(lightcraft_preview::Hash128::parse); + crate::import_move::copy_new(Path::new(src), &dir, &name, expect).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 @@ -784,7 +787,7 @@ impl ImportJob { let reason = "a symbolic link: the file it points to was copied, the link and its target stay"; out.items.push(PreparedItem::Kept(Kept { path: path.clone(), reason: reason.into() })); } - match copy_into(root, &path, &folders, name.as_deref()) { + match copy_into(root, &path, &folders, name.as_deref(), info.content_hash.as_deref()) { Ok(p) => p, Err(e) => { out.items.push(PreparedItem::Failed(path, e)); diff --git a/crates/engine/src/import_move.rs b/crates/engine/src/import_move.rs index 7221c595..3465eaa7 100644 --- a/crates/engine/src/import_move.rs +++ b/crates/engine/src/import_move.rs @@ -171,9 +171,15 @@ pub(crate) fn place(src: &Path, dir: &Path, name: &str) -> Result Result { +/// disk and verified; a bad copy is removed and reported, so a card is never wiped on the strength +/// of it. Returns the new path. +/// +/// `expect` is the content hash the import's probe computed from its full read of the source +/// ([`lightcraft_preview::hash_bytes`]): the copy is read back and checked against it, which spares +/// a third read of the card (issue #134). It also catches a source that changed since the probe +/// (whose recorded metadata and hash would be stale). Without one the copy is compared byte for +/// byte with the source. +pub(crate) fn copy_new(src: &Path, dir: &Path, name: &str, expect: Option) -> 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}")), @@ -181,7 +187,7 @@ pub(crate) fn copy_new(src: &Path, dir: &Path, name: &str) -> Result 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())), @@ -262,12 +268,14 @@ fn link_or_copy(src: &Path, dst: &Path) -> io::Result<()> { Err(_) => {} } } - copy_verified(src, dst) + // Move keeps the byte-for-byte comparison: the source is deleted on the strength of it + copy_verified(src, dst, None) } -/// Copy into a new file, sync it to disk and compare it with the original; on any failure the -/// partial copy is removed. -fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> { +/// Copy into a new file, sync it to disk and verify it: against `expect` (the probe's hash of the +/// source) when given, else byte for byte against the source. On any failure the partial copy is +/// removed. +pub(crate) fn copy_verified(src: &Path, dst: &Path, expect: Option) -> io::Result<()> { let mut out = OpenOptions::new().write(true).create_new(true).open(dst)?; let r = (|| { let mut inp = File::open(src)?; @@ -283,10 +291,14 @@ fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> { if let Ok(m) = fs::metadata(src).and_then(|m| m.modified()) { let _ = out.set_modified(m); } - if !same_bytes(src, dst)? { - return Err(io::Error::other("the copy differs from the original")); + match expect { + Some(h) if hash_file(dst)? != h => { + Err(io::Error::other("the copy differs from the original as it was read for the import (a bad copy, or the file changed meanwhile)")) + } + Some(_) => Ok(()), + None if !same_bytes(src, dst)? => Err(io::Error::other("the copy differs from the original")), + None => Ok(()), } - Ok(()) })(); drop(out); if r.is_err() { @@ -295,6 +307,20 @@ fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> { r } +/// [`lightcraft_preview::hash_bytes`] of a file's content, read in chunks. +fn hash_file(path: &Path) -> io::Result { + let mut f = File::open(path)?; + let mut h = lightcraft_preview::Hasher128::new(); + let mut buf = vec![0u8; 1 << 20]; + loop { + let n = fill(&mut f, &mut buf)?; + if n == 0 { + return Ok(h.finish()); + } + h.update(buf.get(..n).unwrap_or(&[])); + } +} + /// Do two files hold the same bytes? pub(crate) fn same_bytes(a: &Path, b: &Path) -> io::Result { if fs::metadata(a)?.len() != fs::metadata(b)?.len() { diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 7fd8a596..91f72e02 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -830,7 +830,9 @@ pub struct ProbeInfo { pub captured: Option, pub meta: lightcraft_catalog::Meta, pub as_shot_wb: Option<(f64, f64)>, - /// Hash of the file's bytes (hex), for duplicate detection. + /// Hash of the file's bytes (hex), for duplicate detection. When it is a 32-digit + /// [`lightcraft_preview::hash_bytes`] of the whole file (as the native probe computes it), + /// Import → Copy verifies each copy against it instead of reading the source again. pub content_hash: Option, /// Lens corrections embedded in the file (DNG opcodes). pub embedded_lens: Option, diff --git a/crates/engine/src/originals.rs b/crates/engine/src/originals.rs index 59a5f6a2..86128108 100644 --- a/crates/engine/src/originals.rs +++ b/crates/engine/src/originals.rs @@ -68,9 +68,14 @@ impl crate::Session { OriginalGuard::new(&self.catalog) } - /// `Err` when writing `path` would replace a catalogued photo's original or sidecar. + /// `Err` when writing `path` would replace a catalogued photo's original or sidecar. A free + /// path (the usual case) is answered without building the guard over the whole library. pub fn check_write_target(&self, path: &str) -> Result<(), String> { - self.original_guard().check(Path::new(path)) + let path = Path::new(path); + if std::fs::symlink_metadata(path).is_err() { + return Ok(()); // nothing there to replace + } + self.original_guard().check(path) } } diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index dcd74cb4..1515d7bf 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -323,3 +323,24 @@ fn a_failed_export_write_keeps_the_previous_file() { assert_ne!(std::fs::read(&prev).unwrap(), b"last week's export"); let _ = std::fs::remove_dir_all(&dir); } + +/// Issue #134: exports can always be made again, so they are written atomically but not synced to +/// disk file by file; an edit copy that becomes a library photo still is. +#[test] +fn exports_are_atomic_without_a_sync() { + use crate::export::{Conflict, Destination}; + use lightcraft_catalog::safe_file::syncs_on_this_thread; + let (mut s, id, dir, _src, _) = library_with_jpeg("export-nosync"); + let out = dir.join("out"); + let to = Destination { dir: out.to_string_lossy().to_string(), exact: None }; + let o = ExportOptions { conflict: Conflict::Unique, ..Default::default() }; + let before = syncs_on_this_thread(); + for _ in 0..3 { + disk_batch(&mut s, id, &o, &to).unwrap(); + } + assert_eq!(syncs_on_this_thread(), before, "no per-file sync for exports"); + assert_eq!(std::fs::read_dir(&out).unwrap().count(), 3, "three exports, no temp files"); + crate::export::write_file_durable(&out.join("edit.tif").to_string_lossy(), b"tiff").unwrap(); + assert!(syncs_on_this_thread() > before, "the durable writer syncs"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/engine/src/tests_import_move.rs b/crates/engine/src/tests_import_move.rs index 5eb278fe..76ea0a3d 100644 --- a/crates/engine/src/tests_import_move.rs +++ b/crates/engine/src/tests_import_move.rs @@ -207,6 +207,67 @@ fn copy_import_verifies_each_copy() { let _ = std::fs::remove_dir_all(&base); } +/// Copy verifies against the probe's content hash (issue #134): the hash, not a re-read of the +/// source, is the reference — a copy that matches the source but not the hash is refused, a +/// corrupted copy is caught, and the file hash agrees with `hash_bytes` across read chunks. +#[test] +fn copy_is_verified_against_the_probe_hash() { + use crate::import_move::copy_verified; + let base = temp_dir("copyhash"); + let src = base.join("src.bin"); + // larger than one 1 MB read, not a multiple of it + let bytes: Vec = (0..(3 << 20) + 12_345u32).map(|i| (i.wrapping_mul(2_654_435_761) >> 13) as u8).collect(); + std::fs::write(&src, &bytes).unwrap(); + let good = lightcraft_preview::hash_bytes(&bytes); + copy_verified(&src, &base.join("ok.bin"), Some(good)).unwrap(); + assert_eq!(std::fs::read(base.join("ok.bin")).unwrap(), bytes); + // a hash that doesn't match: refused (a byte compare with the source would have passed) + let stale = lightcraft_preview::hash_bytes(b"what the probe read earlier"); + let e = copy_verified(&src, &base.join("stale.bin"), Some(stale)).unwrap_err(); + assert!(e.to_string().contains("differs"), "{e}"); + assert!(!base.join("stale.bin").exists(), "the refused copy is removed"); + // a copy that comes out different: caught by the hash + inject(Fault::Corrupt); + let r = copy_verified(&src, &base.join("bad.bin"), Some(good)); + inject(Fault::None); + assert!(r.is_err()); + assert!(!base.join("bad.bin").exists()); + assert_eq!(std::fs::read(&src).unwrap(), bytes, "the source is untouched"); + let _ = std::fs::remove_dir_all(&base); +} + +/// A file replaced after Review Import probed it is not imported on the strength of the old probe: +/// its copy no longer matches the probe's hash, so it's reported as failed and nothing is left. +#[test] +fn copy_import_refuses_a_file_changed_since_the_review() { + let base = temp_dir("copychanged"); + let (card, dest) = (base.join("card"), base.join("out")); + write_png(&card.join("a.png"), 3); + let mut s = session(); + let r = s.execute("library.importPreview", &json!({"paths": [card.to_string_lossy()]})).unwrap(); + assert_eq!(r["scanned"], 1, "{r}"); + write_png(&card.join("a.png"), 9); // other pixels + let r = s + .execute( + "library.import", + &json!({"paths": [card.to_string_lossy()], "mode": "copy", "destination": dest.to_string_lossy(), "organize": "flat"}), + ) + .unwrap(); + assert_eq!(len(&r, "imported"), 0, "{r}"); + assert_eq!(len(&r, "failed"), 1, "{r}"); + assert!(r.to_string().contains("changed meanwhile"), "{r}"); + assert_eq!(files_under(&dest), Vec::::new()); + // importing again probes afresh and succeeds + let r = s + .execute( + "library.import", + &json!({"paths": [card.to_string_lossy()], "mode": "copy", "destination": dest.to_string_lossy(), "organize": "flat"}), + ) + .unwrap(); + assert_eq!(len(&r, "imported"), 1, "{r}"); + 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] @@ -320,3 +381,55 @@ fn move_into_the_library_is_saved_before_sources_go() { assert_eq!(photo_paths(&again), [want.to_string_lossy()]); let _ = std::fs::remove_dir_all(&base); } + +/// Timing for issue #134 (not a pass/fail test): `cargo test -p lightcraft-engine --release +/// write_policy_timing -- --ignored --nocapture`. Writes N export-sized files durably (temp + +/// sync + rename) and atomically only, and copies N files verified byte for byte and against the +/// probe hash, alternating so a loaded machine affects both alike. `LIGHTCRAFT_BENCH_DIR` puts +/// the files on another volume (a USB drive or a NAS is where the difference shows). +#[test] +#[ignore] +fn write_policy_timing() { + use crate::import_move::copy_verified; + use lightcraft_catalog::safe_file::{write_atomic, write_atomic_nosync}; + use std::time::{Duration, Instant}; + let root = std::env::var_os("LIGHTCRAFT_BENCH_DIR").map(PathBuf::from).unwrap_or_else(std::env::temp_dir); + let base = root.join(format!("lc-write-policy-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + let data = |n: usize, seed: u32| -> Vec { (0..n as u32).map(|i| (i.wrapping_add(seed).wrapping_mul(2_654_435_761) >> 11) as u8).collect() }; + let (n, size) = (200usize, 300 << 10); + let (mut synced, mut unsynced) = (Duration::ZERO, Duration::ZERO); + for i in 0..n { + let b = data(size, i as u32); + let t = Instant::now(); + write_atomic(&base.join(format!("sync-{i}.jpg")), &b).unwrap(); + synced += t.elapsed(); + let t = Instant::now(); + write_atomic_nosync(&base.join(format!("nosync-{i}.jpg")), &b).unwrap(); + unsynced += t.elapsed(); + } + println!("export {n} × {} KB: write_atomic (synced) {synced:?}, write_atomic_nosync {unsynced:?}", size >> 10); + let (n, size) = (40usize, 8 << 20); + let card = base.join("card"); + std::fs::create_dir_all(&card).unwrap(); + let hashes: Vec<_> = (0..n) + .map(|i| { + let b = data(size, 7 * i as u32); + std::fs::write(card.join(format!("{i}.raw")), &b).unwrap(); + lightcraft_preview::hash_bytes(&b) + }) + .collect(); + let (mut bytewise, mut hashed) = (Duration::ZERO, Duration::ZERO); + for (i, h) in hashes.iter().enumerate() { + let src = card.join(format!("{i}.raw")); + let t = Instant::now(); + copy_verified(&src, &base.join(format!("bytes-{i}.raw")), None).unwrap(); + bytewise += t.elapsed(); + let t = Instant::now(); + copy_verified(&src, &base.join(format!("hash-{i}.raw")), Some(*h)).unwrap(); + hashed += t.elapsed(); + } + println!("copy {n} × {} MB: byte compare with the source {bytewise:?}, probe hash {hashed:?}", size >> 20); + let _ = std::fs::remove_dir_all(&base); +} diff --git a/docs/parity.md b/docs/parity.md index cfc61c96..4eb48c4e 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -114,7 +114,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); the import itself (probing, sidecars, copying / moving files) runs on a worker thread and its batches join the library between frames, with a progress window and Cancel; one undo step. Drag-and-drop goes through the same background import | | 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); 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-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 (a new file, synced, checked against the content hash the scan computed — byte compare with the source when there is none, issue #134; 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 | @@ -434,7 +434,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-EXP-METADATA | Metadata policy | P1 | ✅ | `cmd:app.export` (`metadata`, `removeLocation`) | | | LR-EXP-WATERMARK | Watermark | P1 | ✅ | `cmd:app.export` (`watermark`: text or `image` + `imageWidth`), `crates/engine/src/export.rs` (`Watermark`) | text (size, colour, shadow) or a graphic with transparency (width as % of the photo, converted to the output colour space); position, inset, opacity | | LR-EXP-NAMING | File naming | P1 | ✅ | `cmd:app.export` (`naming`, `startNumber`), `crates/engine/src/rename.rs` (`expand_tokens`) | one template language for export, Rename Photos and import renaming: `{name}`, `{num}` (number at the end of the name), `{seq}` / `{seq:N}`, `{date}` / `{date:%Y-%m-%d}`, `{folder}`, `{camera}`, `{lens}`, `{iso}`, `{rating}`, `{title}`, `{creator}`, `{ext}`; free-form template rather than a list of named schemes | -| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file, synced and renamed into place (issue #93) | +| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file and renamed into place (issue #93; not synced file by file — exports can be made again, issue #134) | | 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 | diff --git a/docs/whats-new.md b/docs/whats-new.md index 701600ef..5ae3fd53 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -14,18 +14,25 @@ the same name and "Overwrite" (or Export with Previous repeating it), an exact output path from the control channel or MCP, a merge preview path or `lightcraft-cli render IMG.jpg -o IMG.jpg` is refused with a clear message, and the original is left byte for byte. Ordinary earlier exports are still overwritten when asked. - Exported files are written to a temp file, synced and then renamed into place, so a full disk or an unplugged - drive never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists" - choice too. + Exported files are written to a temp file and then renamed into place, so a full disk or an unplugged drive + never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists" choice too. - Convert to DNG, Copy as DNG, Photo Merge and smart previews no longer write straight to the final file (issue #106): a DNG is checked against the raw data, written to a temp file, synced and read back before it gets its name (never replacing a file), and only then is the photo relinked or the raw copy removed — a failed write leaves no DNG and keeps the raw. Smart previews are written the same way; a damaged one (cut short by a crash or a full drive) no longer counts as built and Build Smart Previews replaces it. - 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. + checked against the content read from 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. +- Faster exports and card imports, with the same protection for what can't be recreated (issue #134): exports, + renders and screenshots are still written to a temp file and renamed into place, but no longer forced to disk + one by one — they can always be exported again, and on a USB drive or a NAS that per-file sync dominated a large + export. The catalog, XMP sidecars, settings, DNGs, merges, smart previews and the copies an import makes are still + synced. Import ▸ Copy now checks each copy against the content hash taken while scanning the card instead of + reading the card a third time (Move still compares byte for byte before it deletes a source); a file that changed + on the card after Review Import is reported instead of being imported with stale details. Checking an export path + against the library's originals no longer scans the whole library when nothing is at that path. - Saving metadata to an XMP sidecar another application wrote no longer replaces it (issue #92): LightCraft merges its fields in and keeps the rest — e.g. that application's develop settings and edit history — byte for byte. A sidecar that isn't valid XMP is copied to `.xmp.bak-