Skip to content

fix(select): a picked picture that is edited away no longer leaves its highlight behind - #2810

Open
hade0ntema wants to merge 2 commits into
storytold:mainfrom
hade0ntema:fix/deleted-image-ghost
Open

hade0ntema wants to merge 2 commits into
storytold:mainfrom
hade0ntema:fix/deleted-image-ghost

Conversation

@hade0ntema

Copy link
Copy Markdown

What this changes

With the Select tool, clicking a picture picks it and draws a blue highlight over it (image_pick). The pick was only cleared by Escape, a drag/double click, or another click. So: pick a picture, open Edit a PDF ▸ Edit text & images, delete the picture, and its highlight stays on the now-empty spot of the page. There is nothing left under it to click, so the user cannot remove it.

The pick now stores the document's edit generation (PickedImage::generation) and is dropped as soon as the document has been edited since (delete, move, Undo/Redo). image_pick::drop_stale runs once per page per frame right before the highlight is painted. This is the same condition under which Ctrl+C already refused a stale pick ("The page has changed; select the image again").

I searched open and closed PRs and issues for this before opening it (delete image, deleted image remains, selection box remains, blue rectangle): nothing reports it. #894 is about Optimize dropping an unused deleted image, which is unrelated.

How you verified it

  • cargo fmt --check
  • cargo clippy -p pdfcraft-ui-egui --all-targets -- -D warnings: clean
  • cargo test -p pdfcraft-ui-egui --test viewer: 52 passed (includes the new test)
  • cargo test -p pdfcraft-ui-egui --test editing editing_existing: 3 passed (image/artwork delete in Edit mode unchanged)

Tests. New: a_picked_picture_that_is_deleted_in_edit_mode_loses_its_highlight in tests/viewer.rs. It picks the picture with the Select tool, enters Edit text & images, clicks the picture and presses Delete, then asserts the picture is gone from page_images and picked_image is None.
Seen failing before the fix: I stashed the fix and ran it. It failed on the last assertion with left: Some(PickedImage { page: 0, index: 0, rect: [0.0, 0.0, 40.0, 30.0] }), right: None, then passed with the fix restored.

Never-crash

  • No new unwrap/expect/panic! outside tests, no indexing, no unsafe, no arithmetic or allocation on input.

Provenance

  • No assets, fonts or copied code.

Security

  • No change to --root confinement, MCP, the control channel or network access.

Parity and reach

Not applicable: a UI state bug, nothing new to expose to automation.

Anything a reviewer should look at first

Any edit now drops the pick, including an unrelated one (for example adding a comment elsewhere). I preferred that to keeping an index that may point at a different picture after the edit; the user just clicks the picture again. Reporter's flow, reproduced on Windows; not run on macOS/Linux.

🤖 Generated with Claude Code

…s highlight behind

With the Select tool a click picks a picture and draws a blue highlight over it. The pick was
only cleared by Escape, a drag or another click. Picking a picture, then entering Edit text &
images and deleting it left the highlight on the empty spot, with nothing left to click to
remove it.

The pick now remembers the document's edit generation and is dropped once the document has been
edited since (a delete, a move, Undo), the same condition under which the copy shortcut already
refused it as stale.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at head da48b84b8cb00de3cb2bd045c8e41694db1fb649 (tree b1357e500c60f378b88f9bd6cd0497fc63a03a2b, main f0c9901c398).

Source review: the pick now carries the document's edit generation, and drop_stale runs once per page per frame before paint, so a delete, move or Undo/Redo clears the highlight. page_input stamps the generation at pick time, so a fresh pick is never dropped. No new unwrap, indexing or unsafe. Dropping the pick on any later edit is a deliberate trade-off that is documented in the description, and it is the safe side.

Independently run on a Linux validation host (Rust 1.95, 2 jobs, 20 GiB no-swap limit, source hashes identical before and after):

  • cargo fmt --all -- --check: pass
  • cargo test --locked -p pdfcraft-ui-egui --test viewer: 52 passed, 0 failed (includes the new test)
  • cargo test --locked -p pdfcraft-ui-egui --test editing editing_existing: 3 passed
  • cargo clippy --locked -p pdfcraft-ui-egui --all-targets -- -D warnings: clean
  • cargo xtask layers: pass
  • Red check: with crates/ui-egui/src reverted to main and the new test kept, a_picked_picture_that_is_deleted_in_edit_mode_loses_its_highlight fails on the last assertion (left: Some(PickedImage { page: 0, ... }), right: None).

Not verified: macOS and Linux desktop behaviour (the author tested on Windows only), and the hosted CI (no checks were reported on this head when I looked).

…next frame

The first fix dropped a stale pick while painting the next frame. The window only redraws on
the next input, so after deleting a picture in Edit text & images its highlight stayed on screen
until the user clicked. Clear the pick in apply_edit, in the same step as the edit, and ask for a
repaint. drop_stale stays for the paths that change the document without apply_edit (Undo, Redo).

An edit-level test checks the pick right after apply_edit with no frame in between; it fails
without the change. The earlier one-frame test could not tell, the test harness redraws more
often than the app does.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@hade0ntema

Copy link
Copy Markdown
Author

Update after testing the build by hand on Windows: with the first commit the highlight was still on screen after deleting the picture until the next click, because the pick was only dropped while painting the following frame and the window redraws on input. The new commit (37a9346) clears the pick inside apply_edit and requests a repaint; drop_stale stays for Undo/Redo. Added an_edit_clears_the_picked_picture_at_once, which checks the pick right after apply_edit with no frame in between and fails without the change (the one-frame test I had before could not tell: the harness redraws more than the app). viewer: 53 passed, clippy and fmt clean. Sorry for the second round after the approval.

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 37a9346. Clearing picked_image immediately after a successful session.apply and requesting repaint closes the stale highlight gap found after the earlier approval. The new no-frame assertion tests the edit-time state directly instead of relying on the harness to redraw.

Independent Linux validation at this exact head passed: immediate-clear and one-frame delete regressions (one each), viewer53, editing129 with4 ignored, object-selection11, edit-engine156, formatting and affected strict Clippy for all targets. Focused regressions overlap the viewer suite; these counts are not a disjoint total. The four ignored tests require private fixtures/fonts. All before/after tracked-source hashes matched and the checkout remained clean. No native desktop interaction test was performed. Hosted CI remains action_required with no jobs observed, not a green hosted result. No remaining blocking source finding in this scoped update.

@aiongg aiongg self-assigned this Oct 16, 2026
aiongg added a commit to aiongg/pdfcraft that referenced this pull request Oct 16, 2026
… away no longer leaves its highlight behind

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants