Conversation
Replace gtk::Picture + DrawingArea with ZoomableCanvas, a custom gtk::Widget subclass that renders via append_scaled_texture (GPU) and uses Cairo only for lightweight overlay primitives. Zoom: Ctrl+scroll at cursor, pinch-to-zoom, +/- keys, 0/Home reset. Pan: middle-drag or Ctrl+left-drag when image exceeds viewport. Selection and OCR highlights stay aligned at all zoom levels. Core ViewTransform (geometry.rs) stores canonical (zoom_factor, center_img) state in image coordinates — resize-stable, no pan drift. Dynamic max zoom ensures 1:1 pixel scale is always reachable.
Summary by CodeRabbit
WalkthroughAdds a ZoomableCanvas widget and related UI gestures (Ctrl+scroll, pinch, drag, keyboard zoom/reset) plus a new ViewTransform implementation and an OcrWordIndex spatial index to support zoom-aware rendering, coordinate transforms, and efficient OCR hit-testing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas as ZoomableCanvas
participant Transform as ViewTransform
participant Renderer as GTK_Snapshot
User->>Canvas: Ctrl+Scroll / Pinch (zoom) or Middle-drag (pan)
Canvas->>Canvas: compute zoom_delta or pan_delta, determine anchor
Canvas->>Transform: from_center(widget_w,widget_h,image_w,image_h, new_zoom, anchor)
Transform-->>Canvas: ViewTransform(scale, offset_x, offset_y)
Canvas->>Canvas: update state (scale, offset), clamp_center if needed
Canvas->>Renderer: queue snapshot / append_scaled_texture with transformed bounds
Renderer-->>User: rendered frame with OCR overlays and selection visuals
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/quickview-core/src/geometry.rs (1)
53-67: Consider clarifyingcontain’s tuple return semantics.
(f64, Point)is easy to misread. A named return type (or method docs) would make it explicit that thePointis widget-center coordinates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/quickview-core/src/geometry.rs` around lines 53 - 67, The contain function returns an unnamed tuple (f64, Point) which is confusing—clarify by introducing a named return type (e.g., a struct like ContainResult { scale: f64, center: Point }) or add clear doc comments on contain; update the function signature from pub fn contain(...) -> (f64, Point) to return the new ContainResult type and change callers to read .scale and .center (or update docs to state that the Point is widget-center coordinates) so the semantics of the returned Point are explicit; reference the contain function and Point type when making the change.docs/ARCHITECTURE.md (1)
191-191: Use a fully qualified file path for the zoom-anchor reference.Line [191] references
image_overlay.rswithout path context; using the repo path improves discoverability for new contributors.📝 Proposed doc tweak
-**Zoom anchoring**: anchor-preserving math ensures the image point under the cursor (or pinch center) stays fixed after zoom. See `recenter_for_anchor()` in `image_overlay.rs`. +**Zoom anchoring**: anchor-preserving math ensures the image point under the cursor (or pinch center) stays fixed after zoom. See `recenter_for_anchor()` in `crates/quickview-ui/src/widgets/image_overlay.rs`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ARCHITECTURE.md` at line 191, The doc reference to image_overlay.rs is ambiguous; update the sentence mentioning `recenter_for_anchor()` to include the repository-relative path to the source file (e.g., mention the crate/module path or the full repo path such as src/ui/image_overlay.rs or equivalent) so new contributors can locate `recenter_for_anchor()` easily; find the line in ARCHITECTURE.md that mentions `recenter_for_anchor()` and replace `image_overlay.rs` with the fully qualified file path used in the repo.crates/quickview-ui/src/widgets/image_overlay.rs (1)
517-523: Selection hit-testing is a hot path; consider indexing OCR boxes.Line [517] through Line [523] rescans all words on every drag update. A simple grid/R-tree built in
set_ocr_resultwould reduce interaction latency on large OCR datasets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/quickview-ui/src/widgets/image_overlay.rs` around lines 517 - 523, Selection hit-testing currently iterates ocr.words on every drag (the selected calculation using state.ocr and bbox.intersects), causing slowness; build a spatial index (simple uniform grid or an R-tree) when OCR results are set (in set_ocr_result) and store it on the state (e.g., state.ocr_index or attach to the existing state.ocr struct), then change the selection path to query that index for candidate word indices and only run bbox.intersects on those candidates to produce selected; ensure insertion/clear happens in set_ocr_result and that the drag/update code uses the new index instead of scanning ocr.words.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/quickview-core/src/geometry.rs`:
- Around line 45-50: ViewTransform currently exposes public fields allowing
invalid states (scale <= 0 or non-finite values) which break downstream math;
change the type to encapsulate invariants by making fields private and provide a
checked constructor like ViewTransform::new(scale, offset_x, offset_y) that
returns Result<Self, Error> (or implements TryFrom) and validates scale > 0 and
that scale, offset_x, offset_y are finite; expose read-only accessors (scale(),
offset_x(), offset_y()) and provide validated setters or builder methods (e.g.,
with_scale) that enforce the same checks so callers cannot construct or mutate
an invalid ViewTransform.
In `@crates/quickview-ui/Cargo.toml`:
- Line 19: The manifests and scripts install system GTK without enforcing the
crate's required v4_10 API (you added gtk4 = { version = "0.10", package =
"gtk4", features = ["v4_10"] } in Cargo.toml), so update all user-facing
manifests and install commands to require GTK >= 4.10: change references to the
plain "gtk4" package in README.md examples, scripts/bootstrap_arch.sh install
lines, templates/PKGBUILD.stub depends array, packaging/arch/PKGBUILD depends,
and the .github/workflows/ci.yml install steps to use the package manager syntax
that enforces a minimum version (e.g., gtk4>=4.10 on Arch) so systems with older
GTK cannot install and fail at runtime.
In `@crates/quickview-ui/src/widgets/image_overlay.rs`:
- Around line 261-275: In set_texture, clear the stored OCR result to avoid
leaking highlights/text from the previous image: inside the mutable state block
in the set_texture method (where state.texture, state.image_width/height,
zoom_factor, center_img, selecting, panning, pinch_active, and selected_indices
are set/cleared), also set state.ocr = None (and any related OCR-specific fields
if present) before dropping state so the new texture starts with no OCR data.
In `@README.md`:
- Around line 38-40: Update the "Zoom & Pan" README entry to include the Home
key as a reset shortcut by appending "or `Home`" to the reset list (the line
under the "Zoom & Pan" heading that currently reads "`0` to reset"), so the
reset shortcuts mention `0` and `Home` consistently with other documentation in
this PR.
---
Nitpick comments:
In `@crates/quickview-core/src/geometry.rs`:
- Around line 53-67: The contain function returns an unnamed tuple (f64, Point)
which is confusing—clarify by introducing a named return type (e.g., a struct
like ContainResult { scale: f64, center: Point }) or add clear doc comments on
contain; update the function signature from pub fn contain(...) -> (f64, Point)
to return the new ContainResult type and change callers to read .scale and
.center (or update docs to state that the Point is widget-center coordinates) so
the semantics of the returned Point are explicit; reference the contain function
and Point type when making the change.
In `@crates/quickview-ui/src/widgets/image_overlay.rs`:
- Around line 517-523: Selection hit-testing currently iterates ocr.words on
every drag (the selected calculation using state.ocr and bbox.intersects),
causing slowness; build a spatial index (simple uniform grid or an R-tree) when
OCR results are set (in set_ocr_result) and store it on the state (e.g.,
state.ocr_index or attach to the existing state.ocr struct), then change the
selection path to query that index for candidate word indices and only run
bbox.intersects on those candidates to produce selected; ensure insertion/clear
happens in set_ocr_result and that the drag/update code uses the new index
instead of scanning ocr.words.
In `@docs/ARCHITECTURE.md`:
- Line 191: The doc reference to image_overlay.rs is ambiguous; update the
sentence mentioning `recenter_for_anchor()` to include the repository-relative
path to the source file (e.g., mention the crate/module path or the full repo
path such as src/ui/image_overlay.rs or equivalent) so new contributors can
locate `recenter_for_anchor()` easily; find the line in ARCHITECTURE.md that
mentions `recenter_for_anchor()` and replace `image_overlay.rs` with the fully
qualified file path used in the repo.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
.claude/CLAUDE.mdAGENTS.mdCHANGELOG.mdREADME.mdcrates/quickview-core/src/geometry.rscrates/quickview-ui/Cargo.tomlcrates/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.rsdiagrams/architecture.mmddiagrams/state_machine.mmddocs/ARCHITECTURE.mddocs/DECISIONS.mddocs/DEPENDENCIES.mddocs/DEVELOPMENT.mddocs/PHASED_PLAN.md
💤 Files with no reviewable changes (2)
- diagrams/state_machine.mmd
- crates/quickview-ui/src/windows/shared.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🧰 Additional context used
🧬 Code graph analysis (2)
crates/quickview-ui/src/windows/quick_preview.rs (1)
crates/quickview-ui/src/windows/shared.rs (1)
overlay(55-57)
crates/quickview-ui/src/windows/full_viewer.rs (1)
crates/quickview-ui/src/windows/shared.rs (1)
overlay(55-57)
🪛 LanguageTool
docs/PHASED_PLAN.md
[grammar] ~39-~39: Ensure spelling is correct
Context: ...ll) ✅ - Basic UI shell with libadwaita (headerbar, etc.) ✅ Definition of done - Open...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (13)
docs/DEPENDENCIES.md (1)
8-20: No actionable issues in this documentation hunk.CHANGELOG.md (1)
5-12: Clear and useful changelog entry for the new interaction model.docs/DECISIONS.md (1)
138-160: No actionable issues in this decision-document update.diagrams/architecture.mmd (1)
3-27: Diagram text-formatting cleanup looks good and preserves architecture flow.crates/quickview-core/src/geometry.rs (1)
153-256: Strong test coverage for transform math edge cases.Inverse mapping, anchor-preserving zoom, and clamp behavior are all validated.
.claude/CLAUDE.md (2)
54-54: Same shortcut-doc drift as README (Homereset missing).
84-84: The transform-convention note is clear and useful.docs/PHASED_PLAN.md (1)
11-151: Phase progress updates are clear and internally consistent.The completion markers and partial-progress notes align well with the implemented zoom/pan and OCR-overlay milestones.
AGENTS.md (1)
86-86: Onboarding pointer update looks good.The updated implementation pointer accurately reflects the new
ImageOverlayWidget+ZoomableCanvassplit.crates/quickview-ui/src/windows/full_viewer.rs (1)
30-56: Keyboard zoom/reset wiring is clean and consistent.The overlay-based handling for
+/-/0/Homeintegrates well with existing key propagation and keeps navigation/copy shortcuts intact.crates/quickview-ui/src/windows/quick_preview.rs (1)
37-62: Quick Preview keyboard interactions look good.The added zoom/reset shortcuts are well integrated and preserve existing
Esc/Spaceclose andCtrl+Ccopy flows.docs/DEVELOPMENT.md (1)
41-43: UI architecture guidance update is solid.The revised widget responsibility bullets are clear and align with the new rendering/input model.
crates/quickview-ui/src/widgets/image_overlay.rs (1)
897-953: Good targeted test coverage for view math helpers.The new tests around sizing and dynamic max zoom improve confidence in the transform/rendering foundation.
| quickview-core = { path = "../quickview-core" } | ||
|
|
||
| gtk4 = { version = "0.10", package = "gtk4" } | ||
| gtk4 = { version = "0.10", package = "gtk4", features = ["v4_10"] } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate environment/manifests =="
fd -HI 'Cargo.toml|README.md|DEPENDENCIES.md|PKGBUILD|Dockerfile|Containerfile|.*\.ya?ml' -t f
echo
echo "== GTK 4.10 / v4_10 references =="
rg -n -C2 --glob '!**/target/**' 'v4_10|gtk4|GTK4|4\.10|libgtk-4'Repository: Green2Grey2/QuickView
Length of output: 16301
🏁 Script executed:
cat -n .github/workflows/ci.yml | head -100Repository: Green2Grey2/QuickView
Length of output: 3191
Add GTK 4.10+ version constraints to all user-facing build/deployment manifests.
The Cargo.toml correctly requires v4_10 feature for the Rust crate. However, system-level build instructions lack version enforcement:
README.md(lines 47-51): listsgtk4without version constraintscripts/bootstrap_arch.sh(lines 6, 12): uses baregtk4packagetemplates/PKGBUILD.stub(line 10):depends=("gtk4" ...)without versionpackaging/arch/PKGBUILD(line 12): same issue.github/workflows/ci.yml(lines 32, 74): CI job installsgtk4without minimum version
Users installing on systems with GTK < 4.10 will encounter build/runtime failures related to missing append_scaled_texture API. Update manifests to explicitly require gtk4>=4.10 where supported by the package manager (Arch: gtk4>=4.10, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/quickview-ui/Cargo.toml` at line 19, The manifests and scripts install
system GTK without enforcing the crate's required v4_10 API (you added gtk4 = {
version = "0.10", package = "gtk4", features = ["v4_10"] } in Cargo.toml), so
update all user-facing manifests and install commands to require GTK >= 4.10:
change references to the plain "gtk4" package in README.md examples,
scripts/bootstrap_arch.sh install lines, templates/PKGBUILD.stub depends array,
packaging/arch/PKGBUILD depends, and the .github/workflows/ci.yml install steps
to use the package manager syntax that enforces a minimum version (e.g.,
gtk4>=4.10 on Arch) so systems with older GTK cannot install and fail at
runtime.
| **Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0` to reset. Middle-click drag or `Ctrl+left-drag` to pan. | ||
|
|
||
| **OCR Text Selection** — Tesseract runs asynchronously after the image loads. Drag to select recognized words, `Ctrl+C` to copy. Selection stays aligned at any zoom level. |
There was a problem hiding this comment.
Add Home to the documented reset shortcuts.
This line currently omits Home, which is documented elsewhere in this PR as a reset keybinding.
📝 Suggested doc patch
-**Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0` to reset. Middle-click drag or `Ctrl+left-drag` to pan.
+**Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0`/`Home` to reset. Middle-click drag or `Ctrl+left-drag` to pan.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0` to reset. Middle-click drag or `Ctrl+left-drag` to pan. | |
| **OCR Text Selection** — Tesseract runs asynchronously after the image loads. Drag to select recognized words, `Ctrl+C` to copy. Selection stays aligned at any zoom level. | |
| **Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0`/`Home` to reset. Middle-click drag or `Ctrl+left-drag` to pan. | |
| **OCR Text Selection** — Tesseract runs asynchronously after the image loads. Drag to select recognized words, `Ctrl+C` to copy. Selection stays aligned at any zoom level. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 38 - 40, Update the "Zoom & Pan" README entry to
include the Home key as a reset shortcut by appending "or `Home`" to the reset
list (the line under the "Zoom & Pan" heading that currently reads "`0` to
reset"), so the reset shortcuts mention `0` and `Home` consistently with other
documentation in this PR.
…wTransform with error handling - Updated README to specify that GTK4 version 4.10 or higher is required. - Modified CI workflow to enforce the minimum GTK4 version during setup. - Enhanced ViewTransform in geometry.rs to include error handling for non-finite and non-positive scale values. - Refactored related methods to utilize the new error handling and updated the usage in image_overlay.rs and other files accordingly. - Added OcrWordIndex to the OCR module for improved indexing functionality.
…heck - ARCHITECTURE.md: Section 7.2 now documents OcrWordIndex spatial index; Section 7.3 documents ContainResult, private fields with getters, and ViewTransformError validation. - CHANGELOG.md: add spatial index, ViewTransform hardening, CI version check. - AGENTS.md: mention spatial index in quickview-core description. - CLAUDE.md: document private ViewTransform API and OcrWordIndex.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
README.md (1)
38-38:⚠️ Potential issue | 🟡 MinorInclude
Homein reset shortcuts for consistency.This still documents only
0for reset, while the feature set includesHomeas well.📝 Proposed doc fix
-**Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0` to reset. Middle-click drag or `Ctrl+left-drag` to pan. +**Zoom & Pan** — `Ctrl+scroll` to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0`/`Home` to reset. Middle-click drag or `Ctrl+left-drag` to pan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 38, Update the "Zoom & Pan" documentation line to include the Home key as an alternate reset shortcut: locate the "Zoom & Pan" paragraph (the text containing "Ctrl+scroll to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-` keys, `0` to reset") and change the reset portion to mention both `0` and `Home` (e.g., "`0`/`Home` to reset") so the README accurately reflects the feature set.
🧹 Nitpick comments (1)
crates/quickview-core/src/ocr/index.rs (1)
88-114: Return intersecting indices in deterministic order.Result ordering currently depends on bucket traversal/cell layout. Sorting before return keeps behavior stable across cell sizes and matches the linear fallback path’s predictable ordering.
Proposed fix
- out + out.sort_unstable(); + out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/quickview-core/src/ocr/index.rs` around lines 88 - 114, The function currently collects intersecting word indices into the local Vec out in a bucket-traversal order that varies with grid layout; to make results deterministic and match the linear fallback, sort the out Vec before returning it (e.g., call out.sort_unstable() or out.sort() just before the final return). Locate the return site that returns the local variable named out (in the loop using self.buckets, self.seen, words, gen) and insert the sort call so the returned indices are in ascending, stable order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/quickview-core/src/geometry.rs`:
- Around line 141-165: Change from_center to return Result<Self,
ViewTransformError> instead of panicking: validate inputs (e.g., ensure
center_img.x/y are finite and computed scale/offsets are finite) and propagate
the error from ViewTransform::new rather than calling .expect(...). Locate
from_center which calls Self::contain(...) (use contain.contain_scale and
contain.widget_center) and replace the final Self::new(scale, offset_x,
offset_y).expect(...) with returning the Result produced by ViewTransform::new
(or map/convert its error to ViewTransformError), returning Err early on invalid
inputs.
- Around line 207-223: The clamp_center function currently only guards values
with <= 0.0, which misses NaN/Inf; update clamp_center to first verify all float
inputs are finite (widget_w, widget_h, image_w, image_h, scale, and
center_img.x/center_img.y) using is_finite() and return center_img early if any
are not finite, before performing arithmetic like half_view_w = widget_w / (2.0
* scale) or using clamp; ensure the same finite checks cover both the branch
conditions (image_w * scale <= widget_w / image_h * scale <= widget_h) and the
final clamp calls so no NaN/Inf can be used as bounds or propagated.
In `@crates/quickview-core/src/ocr/index.rs`:
- Around line 138-140: The code currently skips boxes with bbox.w == 0.0 or
bbox.h == 0.0; change the guard so only boxes with negative or non-finite
dimensions are dropped (e.g., use bbox.w < 0.0 || bbox.h < 0.0 or
!bbox.w.is_finite() || !bbox.h.is_finite()), allowing zero-area (degenerate)
boxes to be inserted into the grid index so they match the degenerate-rectangle
query semantics; locate the early-return using the bbox variable in the OCR
grid-index insertion logic and update that condition accordingly.
In `@crates/quickview-ui/src/widgets/image_overlay.rs`:
- Around line 271-279: The image load/reset path in set_texture leaves
last_cursor_widget populated, causing the first Ctrl+scroll to use a stale
anchor; update set_texture (the function where state is reset, around the block
setting state.center_img, state.selecting, state.panning, state.pinch_active,
and state.selected_indices) to also clear the last_cursor_widget field (set it
to None/empty) on texture change so the new image starts with no previous cursor
anchor.
In `@scripts/bootstrap_arch.sh`:
- Around line 11-13: The error message uses command substitution directly in the
echo which can be empty or print pkg-config stderr separately; update the if
block that checks pkg-config --atleast-version=4.10 gtk4 to store the gtk4
version in a variable (e.g., version="$(pkg-config --modversion gtk4 2>/dev/null
|| true)") and then use that variable in the echo, falling back to a clear
message like "not found" when empty; ensure pkg-config stderr is redirected
(2>/dev/null) so the script prints a single robust line and exits with status 1.
---
Duplicate comments:
In `@README.md`:
- Line 38: Update the "Zoom & Pan" documentation line to include the Home key as
an alternate reset shortcut: locate the "Zoom & Pan" paragraph (the text
containing "Ctrl+scroll to zoom at cursor, pinch-to-zoom on touchpad, `+`/`-`
keys, `0` to reset") and change the reset portion to mention both `0` and `Home`
(e.g., "`0`/`Home` to reset") so the README accurately reflects the feature set.
---
Nitpick comments:
In `@crates/quickview-core/src/ocr/index.rs`:
- Around line 88-114: The function currently collects intersecting word indices
into the local Vec out in a bucket-traversal order that varies with grid layout;
to make results deterministic and match the linear fallback, sort the out Vec
before returning it (e.g., call out.sort_unstable() or out.sort() just before
the final return). Locate the return site that returns the local variable named
out (in the loop using self.buckets, self.seen, words, gen) and insert the sort
call so the returned indices are in ascending, stable order.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.claude/CLAUDE.md.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdREADME.mdcrates/quickview-core/src/geometry.rscrates/quickview-core/src/ocr/index.rscrates/quickview-core/src/ocr/mod.rscrates/quickview-ui/src/widgets/image_overlay.rsdocs/ARCHITECTURE.mdpackaging/arch/PKGBUILDscripts/bootstrap_arch.shtemplates/PKGBUILD.stub
🚧 Files skipped from review as they are similar to previous changes (2)
- AGENTS.md
- CHANGELOG.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (9)
.github/workflows/ci.yml (1)
35-35: Good fail-fast guard for GTK4 minimum version.Nice addition: both jobs now enforce
gtk4 >= 4.10before build/lint execution.Also applies to: 78-78
docs/ARCHITECTURE.md (1)
168-196: Architecture notes now match the new zoom/pan implementation well.The transform/index/rendering flow is clearly documented and consistent with the introduced APIs.
packaging/arch/PKGBUILD (1)
12-12: Dependency floor update looks correct.Pinning
gtk4>=4.10here keeps packaging aligned with the UI feature requirements.crates/quickview-core/src/ocr/mod.rs (1)
3-3: Module exposure is appropriate.Publishing
ocr::indexcleanly exposes the new spatial index functionality.templates/PKGBUILD.stub (1)
10-10: Template dependency update is aligned.Good to keep the starter PKGBUILD in sync with the GTK4 minimum requirement.
crates/quickview-core/src/ocr/index.rs (1)
72-107: Good dedupe + bounds hardening in query path.The generation-based
seendedupe and out-of-range index guards are solid defensive choices and prevent duplicate hits or panics under contract violations..claude/CLAUDE.md (1)
54-86: Documentation updates look aligned with the implementation.The added notes for zoom/pan behavior,
ViewTransformusage conventions, andOcrWordIndexhit-testing are clear and consistent with the new architecture.crates/quickview-ui/src/widgets/image_overlay.rs (2)
263-270: Great defensive reset inset_texture.Clearing OCR state and index on texture swap makes the invariant local and avoids stale highlight leakage across images.
525-540: Nice performance fallback strategy for drag-select hit-testing.Using
OcrWordIndex::query_intersectingfirst and falling back to linear intersection when needed is a solid resilience/performance balance.
| pub fn from_center( | ||
| widget_w: f64, | ||
| widget_h: f64, | ||
| image_w: f64, | ||
| image_h: f64, | ||
| zoom_factor: f64, | ||
| center_img: Point, | ||
| ) -> Self { | ||
| // `from_center()` delegates invariants to `ViewTransform::new`. | ||
| // `center_img.x` / `center_img.y` must be finite or `ViewTransform::new` will error. | ||
| debug_assert!( | ||
| center_img.x.is_finite() && center_img.y.is_finite(), | ||
| "from_center: center_img must be finite (x={}, y={})", | ||
| center_img.x, | ||
| center_img.y | ||
| ); | ||
|
|
||
| let contain = Self::contain(widget_w, widget_h, image_w, image_h); | ||
| let scale = | ||
| (contain.contain_scale * zoom_factor.max(f64::MIN_POSITIVE)).max(f64::MIN_POSITIVE); | ||
|
|
||
| let offset_x = contain.widget_center.x - center_img.x * scale; | ||
| let offset_y = contain.widget_center.y - center_img.y * scale; | ||
| Self::new(scale, offset_x, offset_y).expect("ViewTransform invariants violated") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find crates -name "geometry.rs" -type fRepository: Green2Grey2/QuickView
Length of output: 102
🏁 Script executed:
head -20 crates/quickview-core/src/geometry.rsRepository: Green2Grey2/QuickView
Length of output: 460
🏁 Script executed:
sed -n '141,165p' crates/quickview-core/src/geometry.rsRepository: Green2Grey2/QuickView
Length of output: 1063
🏁 Script executed:
rg -A 5 "ViewTransformError" crates/quickview-core/src/Repository: Green2Grey2/QuickView
Length of output: 2210
🏁 Script executed:
rg -B 2 -A 10 "impl ViewTransform" crates/quickview-core/src/geometry.rsRepository: Green2Grey2/QuickView
Length of output: 487
🏁 Script executed:
rg "from_center" crates/quickview-core/src/ -B 2 -A 5Repository: Green2Grey2/QuickView
Length of output: 5154
Avoid panicking from public from_center() on invalid runtime input.
from_center() currently calls .expect(...) which will panic if invariants are violated. Non-finite inputs to center_img, or NaN/Inf results from intermediate calculations, bypass the debug_assert! in release builds and reach ViewTransform::new(), which returns an error. Prefer returning Result<Self, ViewTransformError> to allow proper error propagation.
🔧 Proposed change
- pub fn from_center(
+ pub fn from_center(
widget_w: f64,
widget_h: f64,
image_w: f64,
image_h: f64,
zoom_factor: f64,
center_img: Point,
- ) -> Self {
+ ) -> Result<Self, ViewTransformError> {
// `from_center()` delegates invariants to `ViewTransform::new`.
// `center_img.x` / `center_img.y` must be finite or `ViewTransform::new` will error.
debug_assert!(
center_img.x.is_finite() && center_img.y.is_finite(),
"from_center: center_img must be finite (x={}, y={})",
center_img.x,
center_img.y
);
let contain = Self::contain(widget_w, widget_h, image_w, image_h);
let scale =
(contain.contain_scale * zoom_factor.max(f64::MIN_POSITIVE)).max(f64::MIN_POSITIVE);
let offset_x = contain.widget_center.x - center_img.x * scale;
let offset_y = contain.widget_center.y - center_img.y * scale;
- Self::new(scale, offset_x, offset_y).expect("ViewTransform invariants violated")
+ Self::new(scale, offset_x, offset_y)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn from_center( | |
| widget_w: f64, | |
| widget_h: f64, | |
| image_w: f64, | |
| image_h: f64, | |
| zoom_factor: f64, | |
| center_img: Point, | |
| ) -> Self { | |
| // `from_center()` delegates invariants to `ViewTransform::new`. | |
| // `center_img.x` / `center_img.y` must be finite or `ViewTransform::new` will error. | |
| debug_assert!( | |
| center_img.x.is_finite() && center_img.y.is_finite(), | |
| "from_center: center_img must be finite (x={}, y={})", | |
| center_img.x, | |
| center_img.y | |
| ); | |
| let contain = Self::contain(widget_w, widget_h, image_w, image_h); | |
| let scale = | |
| (contain.contain_scale * zoom_factor.max(f64::MIN_POSITIVE)).max(f64::MIN_POSITIVE); | |
| let offset_x = contain.widget_center.x - center_img.x * scale; | |
| let offset_y = contain.widget_center.y - center_img.y * scale; | |
| Self::new(scale, offset_x, offset_y).expect("ViewTransform invariants violated") | |
| } | |
| pub fn from_center( | |
| widget_w: f64, | |
| widget_h: f64, | |
| image_w: f64, | |
| image_h: f64, | |
| zoom_factor: f64, | |
| center_img: Point, | |
| ) -> Result<Self, ViewTransformError> { | |
| // `from_center()` delegates invariants to `ViewTransform::new`. | |
| // `center_img.x` / `center_img.y` must be finite or `ViewTransform::new` will error. | |
| debug_assert!( | |
| center_img.x.is_finite() && center_img.y.is_finite(), | |
| "from_center: center_img must be finite (x={}, y={})", | |
| center_img.x, | |
| center_img.y | |
| ); | |
| let contain = Self::contain(widget_w, widget_h, image_w, image_h); | |
| let scale = | |
| (contain.contain_scale * zoom_factor.max(f64::MIN_POSITIVE)).max(f64::MIN_POSITIVE); | |
| let offset_x = contain.widget_center.x - center_img.x * scale; | |
| let offset_y = contain.widget_center.y - center_img.y * scale; | |
| Self::new(scale, offset_x, offset_y) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/quickview-core/src/geometry.rs` around lines 141 - 165, Change
from_center to return Result<Self, ViewTransformError> instead of panicking:
validate inputs (e.g., ensure center_img.x/y are finite and computed
scale/offsets are finite) and propagate the error from ViewTransform::new rather
than calling .expect(...). Locate from_center which calls Self::contain(...)
(use contain.contain_scale and contain.widget_center) and replace the final
Self::new(scale, offset_x, offset_y).expect(...) with returning the Result
produced by ViewTransform::new (or map/convert its error to ViewTransformError),
returning Err early on invalid inputs.
| if widget_w <= 0.0 || widget_h <= 0.0 || image_w <= 0.0 || image_h <= 0.0 || scale <= 0.0 { | ||
| return center_img; | ||
| } | ||
|
|
||
| let half_view_w = widget_w / (2.0 * scale); | ||
| let half_view_h = widget_h / (2.0 * scale); | ||
|
|
||
| let center_x = if image_w * scale <= widget_w { | ||
| image_w * 0.5 | ||
| } else { | ||
| center_img.x.clamp(half_view_w, image_w - half_view_w) | ||
| }; | ||
|
|
||
| let center_y = if image_h * scale <= widget_h { | ||
| image_h * 0.5 | ||
| } else { | ||
| center_img.y.clamp(half_view_h, image_h - half_view_h) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "geometry.rs" -path "*/crates/quickview-core/*" 2>/dev/nullRepository: Green2Grey2/QuickView
Length of output: 104
🏁 Script executed:
cat -n crates/quickview-core/src/geometry.rs | head -250 | tail -100Repository: Green2Grey2/QuickView
Length of output: 3659
🏁 Script executed:
wc -l crates/quickview-core/src/geometry.rsRepository: Green2Grey2/QuickView
Length of output: 106
🏁 Script executed:
cat -n crates/quickview-core/src/geometry.rs | sed -n '180,230p'Repository: Green2Grey2/QuickView
Length of output: 1828
🏁 Script executed:
rg -l "clamp_center" -t rustRepository: Green2Grey2/QuickView
Length of output: 151
🏁 Script executed:
rg "clamp_center" -B 5 -A 30Repository: Green2Grey2/QuickView
Length of output: 21460
🏁 Script executed:
ls -la crates/quickview-core/src/Repository: Green2Grey2/QuickView
Length of output: 459
🏁 Script executed:
find . -path "*/crates/quickview-core/src/geometry.rs"Repository: Green2Grey2/QuickView
Length of output: 104
🏁 Script executed:
git ls-files | grep geometryRepository: Green2Grey2/QuickView
Length of output: 102
🏁 Script executed:
find . -name "*.rs" -type f | head -20Repository: Green2Grey2/QuickView
Length of output: 799
🌐 Web query:
Rust f64 clamp NaN infinity behavior documentation
💡 Result:
f64::clamp(min, max) is documented to behave as follows: it returns max if self > max, min if self < min, otherwise returns self—except it “restrict[s] a value to a certain interval unless it is NaN.” In particular: if self is NaN, the result is NaN; if min or max is NaN (or min > max), it panics; and if the clamped result is zero with mixed-sign zeros among self/min/max, the sign of the returned zero (0.0 vs -0.0) is non-deterministic. This definition also implies that ±∞ will clamp to the nearest bound when it lies outside [min, max] (e.g., INFINITY.clamp(min, max_finite) == max_finite). [1]
Source: Rust core primitive f64 docs (f64::clamp). [1]
🏁 Script executed:
rg "widget_w.*widget_h.*image_w" -A 20Repository: Green2Grey2/QuickView
Length of output: 18286
🏁 Script executed:
rg "let half_view_w = widget_w" -B 10 -A 15Repository: Green2Grey2/QuickView
Length of output: 1802
🏁 Script executed:
git log --oneline -5 2>/dev/nullRepository: Green2Grey2/QuickView
Length of output: 146
🏁 Script executed:
ls -la .git 2>/dev/null | head -5Repository: Green2Grey2/QuickView
Length of output: 294
🏁 Script executed:
find . -type f -name "*.rs" 2>/dev/null | wc -lRepository: Green2Grey2/QuickView
Length of output: 67
🏁 Script executed:
find . -type f -name "*.rs" 2>/dev/null | head -20Repository: Green2Grey2/QuickView
Length of output: 799
🏁 Script executed:
find . -maxdepth 3 -type f -name "*.rs" 2>/dev/null | head -10Repository: Green2Grey2/QuickView
Length of output: 47
🌐 Web query:
Rust f64::clamp behavior with NaN infinity non-finite values
💡 Result:
f64::clamp(self, min, max) clamps using IEEE-754 comparisons, except it has special rules for NaN and may be nondeterministic for signed zero:
- If
selfis NaN: returns NaN (it “doesn’t clamp NaN away”). [1] - If
minis NaN ormaxis NaN: panics. [1] - If
min > max: panics. [1] - If values are ±infinity (and bounds are not NaN):
self = +∞→ returnsmax(unlessmaxis also+∞, then returns+∞).self = -∞→ returnsmin(unlessminis also-∞, then returns-∞).- Infinite bounds are allowed; they just affect the comparisons normally. [1]
- Signed zero corner case: if the clamped result is zero and the inputs contain both
0.0and-0.0, the function may return either sign non-deterministically. [1]
Examples (from the docs): NaN stays NaN; and signed-zero sign may vary. [1]
Source: Rust f64::clamp documentation. [1]
[1] (doc.rust-lang.org)
Citations:
🏁 Script executed:
rg "clamp.*NaN\|NaN.*clamp\|is_finite" -t rustRepository: Green2Grey2/QuickView
Length of output: 47
🏁 Script executed:
rg "fn clamp_center\|pub fn clamp_center" -B 5 -A 30Repository: Green2Grey2/QuickView
Length of output: 47
🏁 Script executed:
rg "clamp_center|center_img" -t rust | head -30Repository: Green2Grey2/QuickView
Length of output: 3056
🏁 Script executed:
python3 << 'EOF'
import math
# Simulating the exact code from the review
widget_w = 100.0
widget_h = 100.0
image_w = 500.0
image_h = 500.0
scale = 1.0
center_img_x = float('nan')
center_img_y = float('nan')
# Current guard
guard_check = (widget_w <= 0.0 or widget_h <= 0.0 or image_w <= 0.0 or
image_h <= 0.0 or scale <= 0.0)
print(f"Current guard catches NaN inputs: {guard_check}")
print(f" (All checks are False, so guard doesn't catch NaN)\n")
# Proposed guard with is_finite()
finite_check = (math.isfinite(widget_w) and math.isfinite(widget_h) and
math.isfinite(image_w) and math.isfinite(image_h) and
math.isfinite(scale) and math.isfinite(center_img_x) and
math.isfinite(center_img_y))
print(f"Proposed guard with is_finite() catches NaN inputs: {not finite_check}")
print(f" (center_img values are NaN, so guard would catch it)\n")
# What happens without the fix
def clamp(v, lo, hi):
return min(max(v, lo), hi)
half_view_w = widget_w / (2.0 * scale)
image_w_minus_half = image_w - half_view_w
result = clamp(center_img_x, half_view_w, image_w_minus_half)
print(f"Result of clamp(NaN, {half_view_w}, {image_w_minus_half}) = {result}")
print(f" Is NaN: {math.isnan(result)}")
print(f" Problem: Function would return center_img which is (NaN, NaN)")
EOFRepository: Green2Grey2/QuickView
Length of output: 390
🏁 Script executed:
rg "fn clamp_center" -A 50Repository: Green2Grey2/QuickView
Length of output: 5159
🏁 Script executed:
find . -type f -name "*.rs" 2>/dev/null | xargs grep -l "clamp_center\|half_view_w" | head -3Repository: Green2Grey2/QuickView
Length of output: 155
🏁 Script executed:
rg "let half_view_w" -B 20 -A 20Repository: Green2Grey2/QuickView
Length of output: 2700
clamp_center() must guard against non-finite inputs before arithmetic operations.
Current guard only checks <= 0.0, which cannot catch NaN or Inf values. This creates two failure modes:
- Panic risk: If any dimension parameter is NaN, computing
half_view_w = widget_w / (2.0 * scale)produces NaN. Passing NaN as a clamp bound causes a runtime panic. - Silent corruption: If
center_img.xorcenter_img.yare NaN, they bypass the guard and propagate through clamp as NaN (which returns NaN unchanged), corrupting output.
Add is_finite() checks for all float parameters before any arithmetic:
Proposed fix
- if widget_w <= 0.0 || widget_h <= 0.0 || image_w <= 0.0 || image_h <= 0.0 || scale <= 0.0 {
+ if !widget_w.is_finite()
+ || !widget_h.is_finite()
+ || !image_w.is_finite()
+ || !image_h.is_finite()
+ || !scale.is_finite()
+ || !center_img.x.is_finite()
+ || !center_img.y.is_finite()
+ || widget_w <= 0.0
+ || widget_h <= 0.0
+ || image_w <= 0.0
+ || image_h <= 0.0
+ || scale <= 0.0
+ {
return center_img;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if widget_w <= 0.0 || widget_h <= 0.0 || image_w <= 0.0 || image_h <= 0.0 || scale <= 0.0 { | |
| return center_img; | |
| } | |
| let half_view_w = widget_w / (2.0 * scale); | |
| let half_view_h = widget_h / (2.0 * scale); | |
| let center_x = if image_w * scale <= widget_w { | |
| image_w * 0.5 | |
| } else { | |
| center_img.x.clamp(half_view_w, image_w - half_view_w) | |
| }; | |
| let center_y = if image_h * scale <= widget_h { | |
| image_h * 0.5 | |
| } else { | |
| center_img.y.clamp(half_view_h, image_h - half_view_h) | |
| if !widget_w.is_finite() | |
| || !widget_h.is_finite() | |
| || !image_w.is_finite() | |
| || !image_h.is_finite() | |
| || !scale.is_finite() | |
| || !center_img.x.is_finite() | |
| || !center_img.y.is_finite() | |
| || widget_w <= 0.0 | |
| || widget_h <= 0.0 | |
| || image_w <= 0.0 | |
| || image_h <= 0.0 | |
| || scale <= 0.0 | |
| { | |
| return center_img; | |
| } | |
| let half_view_w = widget_w / (2.0 * scale); | |
| let half_view_h = widget_h / (2.0 * scale); | |
| let center_x = if image_w * scale <= widget_w { | |
| image_w * 0.5 | |
| } else { | |
| center_img.x.clamp(half_view_w, image_w - half_view_w) | |
| }; | |
| let center_y = if image_h * scale <= widget_h { | |
| image_h * 0.5 | |
| } else { | |
| center_img.y.clamp(half_view_h, image_h - half_view_h) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/quickview-core/src/geometry.rs` around lines 207 - 223, The
clamp_center function currently only guards values with <= 0.0, which misses
NaN/Inf; update clamp_center to first verify all float inputs are finite
(widget_w, widget_h, image_w, image_h, scale, and center_img.x/center_img.y)
using is_finite() and return center_img early if any are not finite, before
performing arithmetic like half_view_w = widget_w / (2.0 * scale) or using
clamp; ensure the same finite checks cover both the branch conditions (image_w *
scale <= widget_w / image_h * scale <= widget_h) and the final clamp calls so no
NaN/Inf can be used as bounds or propagated.
| if bbox.w <= 0.0 || bbox.h <= 0.0 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Include zero-area OCR boxes in the grid index.
Line [138] currently drops bbox.w == 0.0 / bbox.h == 0.0, while Line [172] explicitly preserves degenerate-rectangle query semantics. This can cause indexed selection to miss words that the fallback intersection logic would include.
Proposed fix
- if bbox.w <= 0.0 || bbox.h <= 0.0 {
+ if bbox.w < 0.0 || bbox.h < 0.0 {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if bbox.w <= 0.0 || bbox.h <= 0.0 { | |
| return; | |
| } | |
| if bbox.w < 0.0 || bbox.h < 0.0 { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/quickview-core/src/ocr/index.rs` around lines 138 - 140, The code
currently skips boxes with bbox.w == 0.0 or bbox.h == 0.0; change the guard so
only boxes with negative or non-finite dimensions are dropped (e.g., use bbox.w
< 0.0 || bbox.h < 0.0 or !bbox.w.is_finite() || !bbox.h.is_finite()), allowing
zero-area (degenerate) boxes to be inserted into the grid index so they match
the degenerate-rectangle query semantics; locate the early-return using the bbox
variable in the OCR grid-index insertion logic and update that condition
accordingly.
| state.center_img = Point { | ||
| x: state.image_width * 0.5, | ||
| y: state.image_height * 0.5, | ||
| }; | ||
| state.selecting = false; | ||
| state.panning = false; | ||
| state.pinch_active = false; | ||
| state.selected_indices.clear(); | ||
| drop(state); |
There was a problem hiding this comment.
Reset last_cursor_widget when loading a new image.
last_cursor_widget is retained across texture changes, so the first Ctrl+scroll on a newly loaded image can zoom around a stale anchor. Clearing it in set_texture avoids that jump.
Proposed fix
state.panning = false;
state.pinch_active = false;
+ state.last_cursor_widget = None;
state.selected_indices.clear();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state.center_img = Point { | |
| x: state.image_width * 0.5, | |
| y: state.image_height * 0.5, | |
| }; | |
| state.selecting = false; | |
| state.panning = false; | |
| state.pinch_active = false; | |
| state.selected_indices.clear(); | |
| drop(state); | |
| state.center_img = Point { | |
| x: state.image_width * 0.5, | |
| y: state.image_height * 0.5, | |
| }; | |
| state.selecting = false; | |
| state.panning = false; | |
| state.pinch_active = false; | |
| state.last_cursor_widget = None; | |
| state.selected_indices.clear(); | |
| drop(state); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/quickview-ui/src/widgets/image_overlay.rs` around lines 271 - 279, The
image load/reset path in set_texture leaves last_cursor_widget populated,
causing the first Ctrl+scroll to use a stale anchor; update set_texture (the
function where state is reset, around the block setting state.center_img,
state.selecting, state.panning, state.pinch_active, and state.selected_indices)
to also clear the last_cursor_widget field (set it to None/empty) on texture
change so the new image starts with no previous cursor anchor.
| if ! pkg-config --atleast-version=4.10 gtk4; then | ||
| echo "Error: GTK4 >= 4.10 is required. Found: $(pkg-config --modversion gtk4)" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the full content of scripts/bootstrap_arch.sh to see if set -euo pipefail is enabled
head -20 scripts/bootstrap_arch.shRepository: Green2Grey2/QuickView
Length of output: 574
🏁 Script executed:
#!/bin/bash
# Test the actual behavior of command substitution failures under set -euo pipefail
python3 << 'EOF'
import subprocess
import tempfile
import os
# Test 1: Command substitution failure with set -euo pipefail
test_script_1 = """#!/bin/bash
set -euo pipefail
echo "Before substitution"
echo "Result: $(pkg-config --modversion gtk4)"
echo "After substitution"
"""
# Test 2: Command substitution in an echo with set -euo pipefail
test_script_2 = """#!/bin/bash
set -euo pipefail
if ! pkg-config --atleast-version=4.10 gtk4 2>/dev/null; then
echo "Error: GTK4 >= 4.10 is required. Found: $(pkg-config --modversion gtk4)" >&2
exit 1
fi
echo "GTK4 is OK"
"""
# Test 3: Command substitution with fallback
test_script_3 = """#!/bin/bash
set -euo pipefail
if ! pkg-config --atleast-version=4.10 gtk4 2>/dev/null; then
found_ver="\$(pkg-config --modversion gtk4 2>/dev/null || echo 'not installed')"
echo "Error: GTK4 >= 4.10 is required. Found: \${found_ver}" >&2
exit 1
fi
echo "GTK4 is OK"
"""
for i, script in enumerate([test_script_1, test_script_2, test_script_3], 1):
with tempfile.NamedTemporaryFile(mode='w', suffix='.sh', delete=False) as f:
f.write(script)
f.flush()
os.chmod(f.name, 0o755)
print(f"\n=== Test {i} ===")
print(f"Script:\n{script}")
result = subprocess.run(['bash', f.name], capture_output=True, text=True)
print(f"Exit code: {result.returncode}")
print(f"Stdout: {result.stdout}")
print(f"Stderr: {result.stderr}")
os.unlink(f.name)
EOFRepository: Green2Grey2/QuickView
Length of output: 1101
Make the failure message robust when GTK4 is not installed.
When pkg-config --modversion gtk4 fails (e.g., if gtk4 is not installed), the command substitution in the error message will be empty and pkg-config's stderr will be printed separately, resulting in a confusing error output showing "Found: " with no version.
🔧 Proposed fix
if ! pkg-config --atleast-version=4.10 gtk4; then
- echo "Error: GTK4 >= 4.10 is required. Found: $(pkg-config --modversion gtk4)" >&2
+ found_ver="$(pkg-config --modversion gtk4 2>/dev/null || echo 'not installed')"
+ echo "Error: GTK4 >= 4.10 is required. Found: ${found_ver}" >&2
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! pkg-config --atleast-version=4.10 gtk4; then | |
| echo "Error: GTK4 >= 4.10 is required. Found: $(pkg-config --modversion gtk4)" >&2 | |
| exit 1 | |
| if ! pkg-config --atleast-version=4.10 gtk4; then | |
| found_ver="$(pkg-config --modversion gtk4 2>/dev/null || echo 'not installed')" | |
| echo "Error: GTK4 >= 4.10 is required. Found: ${found_ver}" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bootstrap_arch.sh` around lines 11 - 13, The error message uses
command substitution directly in the echo which can be empty or print pkg-config
stderr separately; update the if block that checks pkg-config
--atleast-version=4.10 gtk4 to store the gtk4 version in a variable (e.g.,
version="$(pkg-config --modversion gtk4 2>/dev/null || true)") and then use that
variable in the echo, falling back to a clear message like "not found" when
empty; ensure pkg-config stderr is redirected (2>/dev/null) so the script prints
a single robust line and exits with status 1.
1 participant
Summary
ViewTransformstruct in the core geometry module, updates to UI event handling, and documentation/diagram updates to reflect the new architecture.Major Feature: Zoom & Pan
CHANGELOG.md,README.md,.claude/CLAUDE.md, [1] [2] [3]Core Transform Math
ViewTransformstruct and methods (from_center,image_to_widget,widget_to_image, etc.) ingeometry.rsto handle all coordinate transforms between image and widget space, including clamping and anchor-preserving zoom. Includes comprehensive unit tests. (crates/quickview-core/src/geometry.rs, crates/quickview-core/src/geometry.rsR44-R256)UI Integration
crates/quickview-ui/src/windows/full_viewer.rs,crates/quickview-ui/src/windows/quick_preview.rs, [1] [2] [3] [4]crates/quickview-ui/src/windows/shared.rs, crates/quickview-ui/src/windows/shared.rsL55)Rendering Pipeline & Dependencies
ZoomableCanvaswidget using GTK4's GSK/Snapshot pipeline for efficient GPU-accelerated image rendering, requiring GTK4 v4_10 features. (crates/quickview-ui/Cargo.toml,docs/DECISIONS.md, [1] [2]Check
cargo fmtpassescargo clippypasses