main's 589941a5 (#6600) refuses to replace a read-only file before anything
is written. This branch's test wrote to a 0o444 file and expected the save to
succeed, so the merge of the two failed on Linux:
cannot save .../doc.psd: the file is read-only; the original file was
not changed
No bug on either side: the two changes disagreed about what a read-only save
should do, and main's answer is the deliberate one. The test now asserts the
refusal, that the original is untouched, and that the attribute and mode survive
— which is what it set out to check. The writable cases alongside it still cover
the metadata copy itself.
The windows-latest failure was the roll-up gate reporting this same failure, not
a second one.
No textual conflicts, but the merge did not compile. main's 589941a5 added a
second AtomicFs test impl using copy_permissions, while this branch renames that
trait method to copy_metadata. Git merged both cleanly because they sit in
different parts of the file, giving a tree whose trait and impl disagree:
E0407, E0046 and E0599 in photocraft-format's lib test. That one error is what
failed all four CI jobs.
main's new impl is renamed to copy_metadata to match.
Verified with --features heif, which is what CI builds with and what I had
previously failed to pass locally: clippy --workspace --all-targets --locked
--features heif -- -D warnings is clean, and the workspace tests pass.
* Preferences: the settings fill the resized dialog, buttons at the bottom (#7420)
#5170 made Preferences resizable, but its contents still sized themselves
from the app window (width capped at 1040, settings area at 680 tall), and
the dialog opened at a 960x900 default taller than that. So it opened with
empty space below the buttons, and enlarging it left the settings and the
buttons stuck in the top-left corner.
The settings now take their width and height from the resizable area,
leaving room for the button row, and Preferences opens at 1040x800. The
settings area stays a few points short of the space it is given: egui grows
the area to fit its contents, so asking for more would make the dialog grow
every frame.
* Preferences: a rendered test drags the resize grip and checks the buttons stay in the corner (#7420)
Opens Preferences in a kittest harness, drags its grip wider and then smaller, and checks the button row stays in the frame's bottom-right corner each time and that the dialog keeps its size afterwards. Fails with the sizing from before the fix.
* Preferences resize test: the buttons must also stay inside the frame
A button row past the frame's bottom or right edge gave a negative gap and passed; now each gap must be 0 to 24 pt.
`composite_ms` measures encoding and submitting a composite, not running it: on
a 24 MP refresh it read 7.7 ms for work the GPU spent ~171 ms on, so nothing in
the app could see a regression that moved work onto the GPU.
A timestamp is written on the GPU timeline at the start and end of every command
encoder a refresh submits, and the spans are summed and read back
asynchronously. Nothing waits, so a measurement arrives a frame or two later and
carries the pixel count of the refresh it belongs to. It is reported as a new
`Option<f64>` `perf.gpu_ms`; `composite_ms` keeps its meaning, so its consumers
are untouched. At 24 MP it reports 83.9 ms against a waited wall clock of
88.4 ms, where the app previously said 3.1 ms.
What it leaves out, documented in the module: queue-level uploads, which wgpu
runs in its own command buffer ahead of ours, and the end-of-batch writeback
after the last timestamp. That is ~5 % of the reported time here and
proportional to the composite, against the ~22x of the number it replaces.
Unavailable rather than wrong: without TIMESTAMP_QUERY and
TIMESTAMP_QUERY_INSIDE_ENCODERS the timer is never built and `gpu_ms` is None —
on older drivers, the CPU compositor, wasm, and Apple silicon, whose GPUs
support counter sampling only at stage boundaries. `PHOTOCRAFT_NO_GPU_TIMING`
suppresses both the timer and the feature request.
Pass-boundary timestamps would cover Apple silicon and need only
TIMESTAMP_QUERY, but they capture 70-78 % of the encoder-level span here and
the shortfall grows with document size, so they were measured and rejected.
D3D12 is exercised: device creation, a 100 ns/tick period and the full
write/resolve/map round-trip succeed with no validation errors, including
across a flushed three-buffer refresh. Numbers throughout are from an
integrated Intel GPU on Mesa/Vulkan and a software D3D12 adapter; no vendor
Windows driver and no Apple hardware was tested.
`damage_since` had a case for the Image > Adjustments dialog but none for the
Properties panel's `live_adjust` preview, so every parameter change returned no
damage and recomposited the whole document — `gpu-full` at 24.0 MP.
That is what made #5778 a latency regression at 100 % zoom: it correctly stops
CPU-compositing a 1.5 MP proxy, but the full-size GPU composite replacing it is
per-pixel work and 24 MP of it costs more. The Properties path now gets what the
dialog had: a session-tagged key, the layer's area as damage, and of that only
what the view shows, catching up as the view moves.
Intel UHD P630, 24 MP document, 12 changes, medians:
v0.9.0 70.3 ms 1.5 MP CPU proxy
main 203.1 ms gpu-full, 24.0 MP
this 47.8 ms gpu-rect, 1.10 MP
Byte-identical to the full-refresh base on all 12 changes and across a pan.
25 % zoom and PHOTOCRAFT_CPU_COMPOSE are unchanged.
`live_region` does not narrow by the layer's mask: a reveal-all mask keeps its
coverage in the default pixel and reports empty content bounds, which emptied
the damage and froze the preview. Every adjustment layer has one.
`file.package` absolutised the caller's `dir` and then built `folder`, `document`
and every `links` entry on it with `join`, which always writes `/`. On Windows
that left one string spelt both ways:
`C:\Users\...\pkg/Print Me/Links/art.png`. The control channel takes `/` and
rejects alternate separators, so a reported path could not be handed back to it.
Spell the absolutised directory with `/` as well, keeping a leading `\\`
(verbatim and UNC prefixes, where Win32 does not accept `/`) intact.
Unrelated to this branch's change; main already fails clippy --all-targets -- -D warnings
without it, which is what failed the Windows integration job here. One
clippy::nonminimal_bool site in canvas.rs, taking clippy's own rewrite: the
negation stays outside, so the is_finite guards and their NaN behaviour are
unchanged. A sweep of all 30 workspace members found no other site.
craft-libs#169 is merged, so the fork pin is no longer needed. Pinned to
storytold/craft-libs 82a24b006eefa036b43c09a67fb9a3fbd17ffe63 (main after the merge) rather than the older 12281d07,
which predates resolve_absolute and the Windows colon refusal.
Saving writes a temporary file and renames it over the target, so everything
attached to the old file went with the file that was unlinked. Several of those
are security decisions, so a save quietly undid them: a download lost its Mark
of the Web, an explicit ACL fell back to whatever the folder hands down.
New crates/format/src/atomic/meta.rs, reached from atomic_write through two
AtomicFs methods (copy_permissions became copy_metadata; replace_existing is new
and defaults to rename, so the failure-injecting test file systems are
unchanged):
- Extended attributes (#6603): flistxattr/fgetxattr/fsetxattr through safe
rustix, per attribute, best effort. macOS Finder tags, com.apple.quarantine
(the Mark of the Web's macOS counterpart) and Linux user.* all ride along.
Attributes the system owns refuse to be written and are skipped;
com.apple.decmpfs is skipped deliberately, since it describes storage the new
bytes do not have.
- macOS ACL (#7073): a new macOS-only exacl dependency (MIT).
fcopyfile(COPYFILE_ACL) is an unsafe fn and fclonefileat(CLONE_ACL) is
APFS-only and would mean writing every save as clone-then-truncate; the
reasoning is in docs/decisions.md. A file with no ACL is left alone.
- Windows (#7094, #7096): ReplaceFileW through winsafe's kernel feature (MIT,
no dependencies, a safe wrapper; the workspace forbids writing the FFI call
here). One call keeps the destination's DACL, its named streams including
Zone.Identifier, and its creation time.
- Automation write path (#7095): AuthorizedWorkspace::write copies the replaced
file's attributes through the two cap-std descriptors, never a path, so a
request path still cannot escape the write root. Permissions there remain
#4963; ReplaceFileW is path-based and so is not used on that path.
The order inside copy_metadata matters: attributes and ACL first, permission
bits last. Writing an attribute needs the destination open for writing, so
copying a 0444 mode onto the temporary file first locks you out of your own
temporary file. Covered by a regression test.
Atomicity is not weakened. The metadata step runs on the temporary file before
the swap, never touches bytes, and never fails a save. Windows is the one place
the swap itself changed, and a backup file name is passed because it is a
correctness requirement, not an option: ReplaceFileW without one documents
ERROR_UNABLE_TO_MOVE_REPLACEMENT as leaving the replaced file deleted and the
replacement under its temporary name, which is a failed save that loses the
user's document. With a backup name the same failure leaves both files alone.
ERROR_UNABLE_TO_MOVE_REPLACEMENT_2, the one error that does move things, is
unwound by hand; every other error falls back to the plain rename, i.e. to the
previous behaviour.
THE WINDOWS PATHS ARE UNVERIFIED ON WINDOWS. They were written and reviewed on
a Mac. The #[cfg(windows)] tests -- Zone.Identifier, a second named stream,
icacls output, creation time -- have never executed. They do compile and pass
clippy -D warnings against x86_64-pc-windows-msvc, which is as far as a Mac
gets. CI's Windows job is the first real run, and #7094 in particular (Mark of
the Web surviving a replace) is the whole point of the change and is the part
no one has observed working.
Tested on macOS: a real chmod +a entry and a real Finder tag both survive a
save (checked with ls -le and xattr, and as unit tests), plus a 64 KiB
attribute value, a read-only file, the mode, and the automation path.
Still open: Linux POSIX ACLs are not carried over (system.posix_acl_access is
not reported by listxattr, so it needs its own step; not part of these issues),
and macOS creation time is still reset by a save (that half of #6603 needs
setattrlist, which has no safe wrapper here).
* Security: enforce the automation read boundary on stored smart-filter params
Smart filters store a command id and params inside the document, so a
crafted .pcraft could resolve stored ambient paths (displace mapPath,
color-lookup LUT files) at render time without the authorize gate:
synchronous smart-object rendering has no Session argument, spawned job
threads drop the requesting session's authority, and a trusted UI
render could warm the process-wide stack cache with a stage the gate
never saw.
Thread the requesting session's gate through a thread-local in jobs.rs
(entered around command execution, reinstalled on spawned workers and on
job apply, restored on drop), re-check stored filter commands in
render_in before source resolution and any cache lookup and in
apply_filter_to_surface, and re-authorize layer.smartFilter.setParam
with the complete merged params after merging stored document params —
including edits whose request contains no path and actions.play steps.
Denial is conservative: the document is unchanged and no worker starts.
Regression tests in the automation crate cover rerender, cached-stage
reuse, merged-param edits, action playback, background jobs, and filter
transfer, for both the displace map and color-lookup LUT vectors.
* Review: carry render authority through Session::edit and surface denials
Session::edit is the engine's universal mutation entry (linked-file
refresh, transforms, place); renders reached through it ran with no
authorization scope installed, so stored smart-filter params read
ambiently there even after the execute-path fix. Install the scope in
Session::edit as well, keyed on the session's authorize gate.
apply_filter_to_surface also conflated a denied filter with an unknown
one: authorize_render_command's Result was discarded with .ok()?, so a
denial silently rendered unfiltered pixels and the stage entered the
process-wide stack cache. The signature becomes
Result<Option<Surface>>: Err for denials (callers propagate), None for
ids that aren't pixel filters (pass-through semantics unchanged), and an
unreadable map keeps its deliberate pass-through. apply_smart_filter and
apply_smart_filters propagate instead of cur.clone().
Two tests: a refresh driven outside command execution (Session::edit)
is denied and leaves the document unchanged; a denied render caches no
stage. Nine authorization tests pass, engine suite and strict Clippy
clean, panic hunt green.
* ui_set description: name the layerFilter field main added
The Layers-panel filter dropdown (#5048) added layerFilter to the
control method's UI_SET_FIELDS without extending the ui_set tool's
description, so the drift guard (mcp.rs, ui_set_description_names_every_
accepted_field) fails on Windows CI for any branch built on main. Name
the field with its patch shape: by (kind, name or mode), kinds (a list
of layer kinds), name (matches part of the layer name), mode (a blend
mode name). The 40 MCP tests pass; strict Clippy clean.
* wasm size: compose and algo join cms at opt-level "z" to clear the 24 MiB gate
Main's browser build is 171 KB over the 24 MiB wasm gate in CI
(25,337,343 bytes), so every branch built on current main fails the
web size check. Demote three pixel crates in the wasm-release profile:
- cms: transforms are built once and cached (transform::cached),
so "z" costs little runtime (measured -42 KB).
- compose: the browser app composites per frame on the GPU (crates/gpu,
WebGPU/WebGL2), so the CPU compositor is not the browser's hot path
there (measured -122 KB).
- algo: filters run per pixel on every interactive filter application.
"z" there is the lever of last resort (measured -331 KB), taken
because the gate is enforced in CI; if browser filters feel too slow,
restore 3 here and reclaim the bytes from a feature-gated decoder.
Final size 24,811,949 bytes locally, 354 KB under the gate; the native
profiles are untouched. If a maintainer prefers keeping algo at 3, the
alternative is raising the self-imposed 24 MiB early-warning limit
(Cloudflare's hard cap is 25 MiB) - a policy call in packaging/web/
package.sh, not a code fix.
Keep the full workspace and feature build selection while running the craft-fonts pass only for text, io and ui-egui. Ordinary workspace test coverage remains in the first test pass.
Run workspace tests with pinned cargo-nextest, a CI fail-fast profile and a ten-minute per-test hang limit. Keep cheap checks first and doctests in a separate step so failures propagate on Windows.
Run the existing Clippy, dependency layering and scorecard checks before the test passes so failures are reported earlier.
Preserve the existing commands, test coverage and runtime limits. Linux, Windows, corpus, WASM, web and packaging checks passed.
Keep effect-identity memo lookup, pure hashing and insertion atomic across concurrent render callers. Preserve existing identities and add a contention regression with eight synchronized callers.
* ci: restore test and Clippy gates
* ci: keep browser wasm below size gate
* CI: restore plural catalog and MCP resource test gates
Adapted the catalog, parser, and shared-cache test fixes from PhotoCraft PR #3821 by @bommerts. The source-key test, bundled-catalog test, and MCP resource test pass locally.
* CI: expect saved native document name in CLI test
The native save path intentionally writes the output filename into the reopened bundle. Keep the pre-save Run assertion and assert r.pcraft after reopening.
* CI: compare orientation round trips without saved filenames
Native saves encode the output filename in document name. Check each expected name, then compare the remaining document fields and pixels.
