feat: show file info in headerbar and add right-click copy menu#2
Conversation
Closes Phase 2 (info) and Phase 5 (context menu): - ViewerController exposes connect_file_loaded; load_file emits FileInfo (name, dimensions, size, load_failed) on success and decode failure, replaying the last info on late registration so the initial file shows. - Full Viewer headerbar uses adw::WindowTitle: filename as title, "1920x1080 - 2.4 MB" subtitle via glib::format_size, or "Could not load image" on failure. format_subtitle unit-tested. - ZoomableCanvas installs canvas.copy / canvas.copy-all widget actions; right-click opens a PopoverMenu at the cursor. Copy All Text joins all OCR words in reading order. Enablement tracks selection, OCR arrival, and image switches; popover unparented in dispose. - Clipboard access moved into the widget so Ctrl+C and the menu share one code path; both windows get the menu via the shared canvas. Also: PHASED_PLAN v0.2 (Phase 2/5 done, Phase 6 status corrected, FR-002 and ADR-0004 async/sandboxed-loading tasks tracked) and .gitignore rule for local working notes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
Summary by CodeRabbit
WalkthroughAdds canvas clipboard actions and a context menu, emits ChangesClipboard menu, file metadata, and docs updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ZoomableCanvas
participant Popover
participant Clipboard
User->>ZoomableCanvas: right-click
ZoomableCanvas->>ZoomableCanvas: show_context_menu(x, y)
ZoomableCanvas->>Popover: create or reuse menu
User->>Popover: choose Copy or Copy All Text
Popover->>ZoomableCanvas: activate canvas.copy / canvas.copy-all
ZoomableCanvas->>Clipboard: set_text(if non-empty)
sequenceDiagram
participant FullViewer
participant ViewerController
participant WindowTitle
FullViewer->>ViewerController: connect_file_loaded(callback)
ViewerController->>ViewerController: load_file(...)
ViewerController-->>FullViewer: FileInfo
FullViewer->>FullViewer: format_subtitle(&FileInfo)
FullViewer->>WindowTitle: set_subtitle(text)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cargo deny's advisories check fails on anyhow 1.0.102 (unsound downcast_mut, dtolnay/anyhow#451; fixed in 1.0.103). Unrelated to this branch's changes — any branch would fail CI until the lockfile moves.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/quickview-ui/src/windows/shared.rs`:
- Around line 104-141: The load failure path in load_file leaves the previous
image visible because only ImageOverlayWidget::set_texture updates
state.texture. In the Err(err) branch of load_file, clear the current canvas
texture in addition to resetting OCR state so the overlay matches the new
FileInfo with load_failed=true and the newly selected filename. Use the existing
load_file and ImageOverlayWidget::set_texture flow to locate the right place for
the reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3931d9b-a76f-44e9-8c6c-91ee1c9767b6
📒 Files selected for processing (6)
.gitignorecrates/quickview-ui/src/widgets/image_overlay.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/windows/quick_preview.rscrates/quickview-ui/src/windows/shared.rsdocs/PHASED_PLAN.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: lint
- GitHub Check: build-test
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use rustfmt and clippy for code formatting and linting, as tracked in rust-toolchain.toml
Run clippy with all targets and features enabled with -D warnings flag to treat warnings as errors
Files:
crates/quickview-ui/src/windows/quick_preview.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/windows/shared.rscrates/quickview-ui/src/widgets/image_overlay.rs
🔇 Additional comments (6)
.gitignore (1)
32-34: LGTM!docs/PHASED_PLAN.md (1)
120-120: Phase 6 status still says “partially done.”This conflicts with the updated completion state described in the change notes. If the section is meant to be finalized, update the heading to keep the document internally consistent.
crates/quickview-ui/src/widgets/image_overlay.rs (1)
80-83: LGTM!Also applies to: 107-123, 133-139, 303-326, 343-364, 463-473, 537-537, 618-618, 794-831
crates/quickview-ui/src/windows/shared.rs (1)
17-33: LGTM!Also applies to: 46-49, 65-67, 86-103, 175-177
crates/quickview-ui/src/windows/full_viewer.rs (1)
6-9: LGTM!Also applies to: 19-32, 43-49, 84-145
crates/quickview-ui/src/windows/quick_preview.rs (1)
43-43: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c0deae0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model.append(Some("Copy All Text"), Some("canvas.copy-all")); | ||
|
|
||
| let popover = gtk::PopoverMenu::from_model(Some(&model)); | ||
| popover.set_parent(self); |
There was a problem hiding this comment.
Because ZoomableCanvas is a custom gtk::Widget with no layout manager or size_allocate implementation, parenting a GtkPopoverMenu to it here is not enough for GTK to allocate and position that popover; custom popover parents need to present the popover during size allocation. In the new right-click path this means the context menu can be unallocated, misplaced, or fail to show. Add a size_allocate vfunc that presents the stored popover, or host it under a widget that already supports popover children.
Useful? React with 👍 / 👎.
Addresses PR #2 review findings: - clear_texture() resets the canvas (texture, dims, OCR, selection, view) when a decode fails, so the display matches the load_failed headerbar info instead of showing the previous image (CodeRabbit). - size_allocate now presents the context-menu popover: custom widgets without a layout manager must call gtk_popover_present() during allocation or the popover can be unallocated/mispositioned after window resizes (Codex).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d46b4b570a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.overlay.clear_texture(); | ||
| self.overlay.set_ocr_busy(false); |
There was a problem hiding this comment.
When a decode failure happens after a previous image's OCR job is still running, this branch clears the canvas and returns without advancing ocr_job_id. The pending spawn_local receiver still sees job_id == job_id_cell.get() and can call set_ocr_result for the previous file, re-enabling stale OCR text/Copy All Text while the header reports the new file failed to load. Bump or otherwise invalidate the OCR job id before returning from this failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/quickview-ui/src/widgets/image_overlay.rs (1)
574-643: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
canvas.copyenablement lags during an active selection drag.
on_drag_begin(Line 571) andon_drag_end(Line 652) callupdate_copy_actions(), buton_drag_update— whereselected_indicesis actually mutated whileselecting(Lines 604-637) — never does. Thecanvas.copyaction (and therefore its Ctrl+C keybinding, pershared.rs) will not reflect the live selection until the drag finishes.🛠️ Proposed fix
drop(state); if needs_redraw { self.queue_draw(); + self.update_copy_actions(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/quickview-ui/src/widgets/image_overlay.rs` around lines 574 - 643, The live selection state in on_drag_update is not propagated to the copy action, so canvas.copy stays stale until drag end. Update on_drag_update in image_overlay.rs to call update_copy_actions whenever selected_indices changes during the selecting branch, alongside the existing queue_draw path, so the Ctrl+C binding reflects the current drag selection immediately.
🧹 Nitpick comments (2)
crates/quickview-ui/src/widgets/image_overlay.rs (2)
497-506: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffNo keyboard path to open the context menu.
The menu only opens via right-click (
GestureClickonBUTTON_SECONDARY). Consider also wiring the conventional "Menu" key / Shift+F10 shortcut so keyboard-only users can reach Copy/Copy All Text.Also applies to: 828-850
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/quickview-ui/src/widgets/image_overlay.rs` around lines 497 - 506, The context menu in the image overlay is only reachable through the right-click GestureClick path, so add a keyboard-triggered entry point as well. Update the context menu handling in image_overlay.rs around the existing right_click setup and the related menu code near show_context_menu so the conventional Menu key and Shift+F10 both invoke the same menu logic, giving keyboard-only users access to the Copy and Copy All Text actions.
299-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reset logic between
set_textureandclear_texture.Both methods reset the same eight
CanvasStatefields (image_width,image_height,ocr,ocr_index,zoom_factor,center_img,selecting,panning,pinch_active,selected_indices), diverging only in the texture/dimension source. Consider extracting a sharedreset_view_state(&mut CanvasState)helper to avoid the two copies drifting apart on future changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/quickview-ui/src/widgets/image_overlay.rs` around lines 299 - 338, Both image_overlay::set_texture and image_overlay::clear_texture duplicate the same CanvasState reset steps, so extract that shared state-reset logic into a helper such as reset_view_state on the widget/state to keep behavior consistent. Keep set_texture responsible only for applying the new gtk::gdk::Texture and its dimensions, and let clear_texture reuse the same helper while still clearing the texture-specific fields separately so the two paths cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/quickview-ui/src/widgets/image_overlay.rs`:
- Around line 574-643: The live selection state in on_drag_update is not
propagated to the copy action, so canvas.copy stays stale until drag end. Update
on_drag_update in image_overlay.rs to call update_copy_actions whenever
selected_indices changes during the selecting branch, alongside the existing
queue_draw path, so the Ctrl+C binding reflects the current drag selection
immediately.
---
Nitpick comments:
In `@crates/quickview-ui/src/widgets/image_overlay.rs`:
- Around line 497-506: The context menu in the image overlay is only reachable
through the right-click GestureClick path, so add a keyboard-triggered entry
point as well. Update the context menu handling in image_overlay.rs around the
existing right_click setup and the related menu code near show_context_menu so
the conventional Menu key and Shift+F10 both invoke the same menu logic, giving
keyboard-only users access to the Copy and Copy All Text actions.
- Around line 299-338: Both image_overlay::set_texture and
image_overlay::clear_texture duplicate the same CanvasState reset steps, so
extract that shared state-reset logic into a helper such as reset_view_state on
the widget/state to keep behavior consistent. Keep set_texture responsible only
for applying the new gtk::gdk::Texture and its dimensions, and let clear_texture
reuse the same helper while still clearing the texture-specific fields
separately so the two paths cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 429cab6c-cd6b-48c7-b0ad-ed86e87f75a9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/quickview-ui/src/widgets/image_overlay.rscrates/quickview-ui/src/windows/shared.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/quickview-ui/src/windows/shared.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: build-test
- GitHub Check: lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use rustfmt and clippy for code formatting and linting, as tracked in rust-toolchain.toml
Run clippy with all targets and features enabled with -D warnings flag to treat warnings as errors
Files:
crates/quickview-ui/src/widgets/image_overlay.rs
🔇 Additional comments (3)
crates/quickview-ui/src/widgets/image_overlay.rs (3)
111-127: LGTM on popover lifecycle wiring.
present()insize_allocateandunparent()indispose()are the documented idiomatic patterns for custom widgets that own a popover child.Also applies to: 140-158
63-66: Copy-action enablement plumbing looks solid.
update_copy_actions()is invoked fromset_texture,clear_texture,set_ocr_result,clear_selection,on_drag_begin, andon_drag_end, correctly reflecting selection/OCR state changes across load/reset flows, matching the upstreamclear_texture/set_ocr_resultcontracts inshared.rs.Also applies to: 84-87, 318-337, 340-352, 354-361, 571-572, 652-653, 852-864
828-850: 🎯 Functional Correctness
set_halignis meaningful here.GtkPopoveruseshalignin its placement logic, soset_halign(gtk::Align::Start)is not dead configuration.> Likely an incorrect or invalid review comment.
- Bump ocr_job_id in the decode-failure path so an in-flight OCR job from the previous image can't repopulate the cleared canvas (Codex). - Refresh copy-action enablement during drag updates, not just at drag boundaries (CodeRabbit). - Add keyboard access to the context menu: Menu key and Shift+F10 open it at the pointer (or widget center), per NFR-005 (CodeRabbit). - Extract CanvasState::reset_view_state so set_texture/clear_texture share one reset path instead of duplicating it (CodeRabbit).
1 participant
Summary
Closes out Phase 2 (info display) and Phase 5 (context-menu copy) of the phased plan.
adw::WindowTitle— filename as title,1920×1080 · 2.4 MBas subtitle (glib::format_size), updating immediately on arrow-key navigation. Decode failures show "Could not load image" instead of stale info.ViewerController::connect_file_loadedcallback fired fromload_fileon success and failure; replays the last file's info on late registration so the initial file is covered.canvas.copy/canvas.copy-allwidget actions onZoomableCanvaswith aPopoverMenuat the cursor. Copy is enabled when a selection exists; Copy All Text joins every OCR word in reading order once OCR completes. Both windows (Full Viewer and Quick Preview) get the menu via the shared canvas..gitignorerule for local working notes.Test plan
cargo fmt/cargo clippy --all-targets --all-features -- -D warningscleancargo test --all: 17/17 pass, including 5 new unit tests forformat_subtitle(normal, missing size, 0 bytes, >1 GB, load-failure)