feat: Acrobat UI parity for image editing, text formatting, and UI stabilization and Bug Fixes

[Combiler01]

What this changes

This PR introduces improved default right click UI and features, bug fixes to glitched edit image right click menu, added ability to stretch or shrink image aspect ratios (8 point instead of the old 4), crop functionality has been added and fixed, more robust pdf embedded text editing has been added, locally system installed fonts can also be used now for editing.

1. UI Layout & Interaction Stability

• Fixed context menu collision in Edit text & images option when right clicking an image to edit, overlapping submenus, and pointer event clipping outside bounding boxes.
• Eliminated interaction latency/stutter during layer and transform updates.

  

2. Image Editing & Context Menu (Acrobat Parity)

• Layer Stacking (Arrange Image): Added Bring to Front, Send to Back, Bring Forward, and Send Backward. Modifies the actual PDF content-stream object ordering (not just CSS z-index) with full Undo/Redo and dirty-state tracking.
• Crop & Transform: Replaced whole-page cropping with a 4-point image-specific interactive crop. Upgraded selection to an 8-point bounding box for non-uniform scaling.
• Actions: Added Flip Horizontal/Vertical, Replace, Export, Cut/Copy/Paste aligned with master icons.
• Context menus are now mode-sensitive (only active in Edit Image & Text mode).

3. Advanced Text Formatting & Selection

• Replaced bulky text buttons with a compact, end-of-selection anchored icon toolbar.
• Added formatting tools matching Acrobat: Lists, Underline, Sub/Superscript, Character Spacing, and Line Spacing.
• System Fonts: Integrated local PC font scanning into the font selector.

Better select Menu when selecting texts

Ability to Pin Tools

Anything a reviewer should look at first

For the Z-order layering and cropping logic in images.rs, this actively modifies the PDF content stream rather than relying on UI overlays. I ensured that the ops.splice engine receives the cloned image operator (or the operator wrapped in a vector clipping path for crops) rather than an empty vector to prevent accidental image deletion during stream rewriting.

[FredApps]

Re-reviewed head fde5abb. All seven previously reported runtime reproductions now pass, including the active-CTM arrange case, the shared rectangular-clip case, and the stale-image paste case with a text-only system clipboard. The Remove Crop event-processing wait has also been restored, and its test passes.

Changes are still required:

1. [P1] The WASM build now fails in image paste. crates/ui-egui/src/content_ui.rs:804–805 references arboard::ImageData and arboard::Error in the cfg(target_arch = "wasm32") branch, but arboard is a native-only dependency. cargo check -p pdfcraft-ui-egui --target wasm32-unknown-unknown fails with two E0433 errors on this line. Keep these native clipboard types out of the WASM code path.

2. [P1] Arrange replaces the actual clipping path with its bounding rectangle. crates/edit/src/images.rs:648–690 reconstructs clipping from visible_rect. Dedicated-scope and shared-scope triangle and even-odd-hole tests show that previously excluded points, respectively [170,170] and [150,150], become included after reordering and reopening. A bounding rectangle cannot preserve the path or an even-odd hole, and the clips.len() > 1 guard does not protect single-path geometry. Preserve the complete clip, including its fill rule, or safely decline an extraction that cannot preserve it.

3. [P1] Arrange discards the image's original graphics state, including opacity. crates/edit/src/images.rs:646–690 also replaces a dedicated image q/Q scope with reconstructed q/cm/Do/Q operations, dropping its original state-setting operators. In both dedicated-scope and shared-scope probes, an image painted with /GS0 gs and /ca 0.25 has effective opacity 1.0 after Arrange. This evidence is from the effective PDF operator state after rewriting/reopening, not a pixel-render comparison. Preserve the relevant graphics state or safely decline unsupported extraction.

Validation on this exact head:

• Focused edit image tests: 16 passed, 6 failed. The six failures are the new dedicated/shared triangle, even-odd-hole, and opacity probes above.
• Previous supplemental edit reproductions: 4 passed.
• Previous supplemental UI reproductions: 3 passed.
• Focused UI image tests: 13 passed, 1 ignored, including the restored Remove Crop test.
• WASM check: failed with the two E0433 errors above.

These are focused checks; no full-suite pass is claimed.

[Combiler01]

Another Fix for WASM Build failure [p1], clipping path [p1] and opacity issues [p1]

[FredApps]

I re-tested 0ca27d3. The WASM build now passes, and the triangle, even-odd hole and locally set opacity cases are fixed. I checked that those images really changed stacking order, not just that the clipping operators were still present.

There are still two gaps in Arrange:

1. Some valid images now silently refuse to move. For an unwrapped image, content_units puts Do in its own unit, so scope_start == target. extract_scope_state immediately returns None, and arrange_image returns Ok without changing anything. Both Bring to Front and Send to Back reproduce this after save/reopen. Some nested image-only scopes also return success without moving. This is a regression in the new extraction guard.

2. Opacity is still wrong when it comes from outside the selected scope. With /GS0 gs in an outer shared q/Q group and the image inside its own nested q/Q, the image actually moves but its opacity changes from 0.25 to 1.0. The reverse also happens: moving an opaque image into a destination where /GS0 is active changes it from 1.0 to 0.25. These are effective PDF graphics-state checks after save/reopen, not pixel-render comparisons.

The missing piece is the effective graphics state at both locations. PDF q saves the current state; it does not reset opacity, clipping, blend mode or other inherited properties. Copying operators found inside the nearest scope therefore misses inherited source state and can pick up unrelated destination state.

Please preserve the image's effective state when moving it, accounting for enclosing scopes and the destination. Where that cannot be done safely, return a clear unsupported-operation result that the UI can explain, rather than silently reporting success. Keep the ordinary unwrapped-image case working.

For the regression tests, assert all three: the selected image actually moved relative to another painted object, its effective appearance state stayed the same, and other content was untouched after reopening. Checking that /GS0 or a clip path occurs somewhere in the stream is not enough. Add both inherited-source-opacity and inherited-destination-opacity cases alongside the unwrapped-image tests.

All seven earlier repros and the six previous clipping/local-opacity probes now pass. The final supplemental edit run has 10 passes and 6 failures covering the cases above; the three supplemental UI probes and 13 focused UI image tests pass (one ignored). I would keep the changes request open until these remaining Arrange cases are addressed.

[Combiler01]

Please verify that there are no errors in this PR
Thank You

[Combiler01]

Focusing on the Last of the issues

[FredApps]

Re-tested 8bd5746. The unwrapped-image moves and the simple inherited-opacity cases now work. The opaque image moved into an active transparency state now returns an explicit unsupported-operation error, which is safer than changing its appearance. The earlier triangle, hole and local-opacity checks still pass.

There is one remaining underlying problem: ExtendedGStateTracker remembers only the last gs operator, but PDF graphics-state dictionaries are partial updates. A later dictionary that sets blend mode does not replace the opacity set by an earlier dictionary.

Two independent probes reproduce this after an actual reorder and save/reopen:

• Outer /GSalpha gs sets ca = 0.25; inner /GSblend gs sets only BM = /Multiply. Moving the image outside that scope changes (0.25, Multiply) to (1.0, Multiply).
• The source uses a blend-mode-only dictionary and is fully opaque; the destination has ca = 0.25. The move changes (1.0, Multiply) to (0.25, Multiply). The new guard misses this because the source has some gs, even though it does not set opacity.

Please track the effective dictionary values through q/Q and successive gs updates, rather than treating one gs operator as the complete state. Either reproduce those effective values at the destination or reject combinations you cannot preserve before changing the document. This also needs to cover keys such as blend mode and soft mask, not just alpha.

I ran the supplemental edit tests: 15 pass, these two appearance checks fail, and one older success-only assertion now encounters the intentional unsupported-operation error. These are effective PDF state checks, not pixel-render comparisons. I did not rerun full gates, UI tests or WASM for this head. The changes request should stay open for the two reproducible appearance regressions above.

[Combiler01]

Requesting another review

[FredApps]

Re-reviewed 8be74fd. The previous two cases are addressed: cumulative alpha plus blend mode survives the move, and moving the opaque image into a translucent destination is now rejected without changing the document.

There is still one confirmed appearance-changing case: two different custom soft masks. In a fixture with an outer /GSA and an image-local /GSB, Bring to Front succeeds but emits /GSB gs /GSA gs /Im0 Do. Before the move, B was the effective mask; after saving and reopening, A is. A rendered sample changes from [64,64,64,255] to [191,191,191,255]. These are distinct valid luminosity masks with transparency-group Form streams, not just different resource names for the same mask.

The problem is the combination of replay order and comparison: the inherited A operation is appended after the already-extracted B operation, and EffectiveGState represents every mask dictionary as the same CustomSMask value. That makes visually_differs accept the changed mask.

Please preserve the original effective gs ordering and distinguish the actual resolved soft-mask state, or reject the move before mutation when it cannot be preserved. Add a regression with two different nested custom masks that checks the effective mask after reopen and rendered output. Checking only that both gs names remain in the stream will miss this.

Validation on this exact head: the cumulative-opacity move passes; the destination-opacity rejection preserves content bytes, image count and effective state after reopen. The new soft-mask identity assertion and rendered-pixel assertion both fail. A separate blend-array renderer probe passes, but that was not an Arrange regression test. I have not rerun the full workspace gates on this head. Changes are still needed before approval.

[Combiler01]

Another Fix Related to Graphic State

[FredApps]

Re-reviewed 60925a8. The previous distinct-custom-mask ordering case is fixed: the image actually moves, /GSA precedes /GSB, mask B survives reopening, and the rendered sample remains [64,64,64,255]. The earlier cumulative alpha/blend and clipping probes also pass; unsafe destination-state moves reject without mutation.

[P1] Preserve the CTM at which each soft mask is established (crates/edit/src/images.rs:968–980).
Emitting all state operators before the reconstructed placement matrix changes the coordinate system of a soft mask that was originally set after a cm. A rendered regression on this head starts with q 100 0 0 100 100 100 cm /GSB gs /Im0 Do Q, followed by separate text. Bring to Front succeeds and rewrites the image as q /GSB gs 100 0 0 100 100 100 cm /Im0 Do Q after that text. After saving/reopening, the selected image has the same bounds and mask B identity, but a non-overlapping pixel changes from [191,191,191,255] to [255,255,255,255]. Both identity and scaled destination CTMs reproduce this.
Please retain the source mask-establishment transform when replaying gs, accounting for the destination CTM, or reject before mutation when it cannot be preserved. Moving every gs uniformly before or after the final image matrix cannot handle masks set at different points in the original transform chain. Add a rendered regression with a nonidentity cm before gs, checking actual reordering, unchanged image bounds, and unchanged non-overlap pixels after reopen.

Validation: supplemental edit run had 18 passes plus 2 older success-expecting probes that now encounter the intentional unsupported-operation rejection; image-filtered edit run had 32 passes plus those same two rejection cases. The rendered probe run had 4 passes and 2 failing transformed-mask cases. The same-group Alpha/Luminosity hypothesis did not reproduce corruption and is not a separate finding. UI, WASM and full workspace gates were not rerun for this head.

[Combiler01]

Fix For CTM content for soft Masking

[FredApps]

Re-reviewed 9a02fc6. The soft-mask CTM finding is fixed in the scoped checks: the original identity/scaled-destination cases now actually reorder the image, preserve its bounds and mask identity after reopening, and keep the non-overlapping pixels at [191,191,191,255] and [64,64,64,255]. Rotation, shear, multiple cm operations and multiple gs operations established under different CTMs also passed the additional rendered probes. The earlier nested-mask, cumulative alpha/blend and clipping cases remain passing. I found no remaining appearance failure in these checks.

One readiness check is still red: cargo fmt --all -- --check exits 1 on a pristine checkout of this exact head. Its output includes formatting changes in crates/edit/src/images.rs, crates/edit/src/lib.rs and crates/edit/src/tests.rs, including the new test and an extra blank line. Please run the repository formatter and push the result so the formatting gate can pass.

Validation actually run: supplemental edit filter 21 passed plus two old supplemental expectations that unwrap intentionally rejected moves; image-filtered edit run 35 passed plus those same two expectations; the new author regression 1 passed; four additional affine-state cases passed; final rendered probe suite 10 passed. The two rejected-move expectations are our supplemental tests, not tests shipped in this PR, and a separate rejection-roundtrip probe confirms no document mutation. cargo clippy -p pdfcraft-edit --all-targets -- -D warnings passed. These filters overlap and should not be summed as unique tests. UI, WASM and full workspace gates were not rerun. The prior functional finding is resolved; formatting remains to be corrected before the follow-up is complete.

[Combiler01]

The functional PDF graphics state, matrix tracking, and opacity edge cases are all passing. I've just run cargo fmt --all and pushed the updated formatting to resolve the final failing CI gate. Let me know if we're good to merge!

[Combiler01]

I see my mistake apologies kinda new to rust I only staged the three edit files in my previous commit and left the ui-egui and engine formatting changes unstaged locally. I've now formatted the entire workspace and verified it locally with cargo fmt --all -- --check

[FredApps]

Re-reviewed dd5b7e7. The complete follow-up commit is formatting-only, and the pristine whole-workspace cargo fmt --all -- --check now passes with cargo1.99.0/rustfmt1.10.0-stable. The checkout stayed clean.

My earlier functional findings are resolved in the scoped validation already reported: actual image reordering preserves clipping, cumulative graphics state, distinct soft-mask identity and mask-establishment transforms after save/reopen, including the rendered affine-state regressions. I have no remaining blocking finding from this review, so I am approving the code.

Validation scope remains as documented in the previous reviews. Only formatting was rerun on this formatting-only head; UI, WASM and full workspace gates were not rerun here. Hosted workflows at this head still report action_required, so this approval is not a claim that hosted CI passed or that all merge requirements are satisfied.

[FredApps]

@echelon Ready to merge

[Combiler01]

I have found more bugs on Arrange image which are in process to be fixed after which conflicts will be resolved again

[Combiler01]

The Cl uses this test
-p pdfcraft-ui-egui --test editing
and this fails on my code
error: test failed, to rerun pass -p pdfcraft-ui-egui --test editing
This code also fails on my local machine but what i have researched is that due to my has code that interacts with Windows Clipboard it causes a heap corruption
so I limit the thread
cargo test -p pdfcraft-ui-egui --test editing -- --test-threads=1
and test runs perfectly
and the test passses is it possible to make this change in Cl for testing??

[FredApps]

Yes, the same editing integration binary can be run serially as a focused Windows diagnostic:

cargo test -p pdfcraft-ui-egui --test editing -- --test-threads=1

I checked the newer fork run on current head 1f728fd. Windows job 117410128698 really does terminate the editing executable with 0xc0000374 (STATUS_HEAP_CORRUPTION). That is a native process crash, not a normal Rust assertion failure. Ubuntu succeeds in this attempt. The earlier Linux stale-clipboard assertion was a different attempt/head and should not be confused with this Windows failure.

Your serial local result is useful evidence of a concurrency-sensitive problem, but it does not yet establish that the Windows clipboard is the root cause. Please repeat this exact target on the same Windows CI environment and commit with one thread, and compare against its normal parallel run. Keep other targets/platforms unchanged for that comparison; --lib would test a different set, and serializing the entire workspace would conceal which test needs isolation.

For a lasting fix, isolate the real OS-clipboard regression from the ordinary headless UI tests (or use a deterministic test clipboard), and investigate the native crash rather than treating a green serial run as proof it is fixed. One concrete test-setup issue is that stale_clipboard_plain_text_does_not_paste_image currently ignores errors from both Clipboard::new and set_text, then asserts that plain text was installed. Make that prerequisite explicit so an unavailable/busy clipboard is not silently mistaken for the intended test state. This observation alone does not prove the heap-corruption cause.

The upstream run is separately awaiting workflow approval and has no jobs, so the fork result is the available execution evidence. I have not changed CI, run a fresh Windows test locally, or re-approved this PR's current head.

[skylartaylor]

Thanks for chasing down the Windows crash. You were right that the clipboard is behind it. I've pushed a fix to your branch (558a4c1), and we'll take the PR from here, so please hold off on pushing further changes for now.

What was going wrong: the canvas called can_paste_image() on every frame, which opened the OS clipboard about 60 times a second. With tests running in parallel, many threads opened the Windows clipboard at once, which corrupted the heap. The fix reads the clipboard only when a right-click opens a menu, sends every clipboard access through a lock so threads take turns, and stops the stale-clipboard test from writing to the real clipboard. That means CI runs the tests in parallel again, without --test-threads=1.