FEAT(client): Enhance image viewer with zoom and pan capabilities#7210
Open
nixietab wants to merge 1 commit into
Open
FEAT(client): Enhance image viewer with zoom and pan capabilities#7210nixietab wants to merge 1 commit into
nixietab wants to merge 1 commit into
Conversation
WalkthroughThis PR refactors 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 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 `@src/mumble/widgets/ResponsiveImageDialog.cpp`:
- Around line 68-70: ResponsiveImageDialog currently calls
m_view->fitInView(...) on every QEvent::Resize in
ResponsiveImageDialog::eventFilter, which resets user zoom/pan; add a boolean
member (e.g., m_autoFitEnabled) initialized true (set true after show) and
change the resize branch to call fitInView only when m_autoFitEnabled is true;
additionally, in eventFilter detect user-initiated zoom/pan inputs (wheel events
that call m_view->scale(...), mouse press/move for panning, or gesture events)
and set m_autoFitEnabled = false when those occur so further resizes won’t
override the user’s view (optionally expose a method to re-enable auto-fit).
- Around line 59-67: The wheel handler in ResponsiveImageDialog::eventFilter
currently applies m_view->scale(…) unbounded on each QWheelEvent
(angleDelta().y()), so repeatedly zooming can make the view unusable; change it
to read the current scale from m_view->transform() (e.g. transform().m11()),
define sensible minScale and maxScale constants, compute the proposed new scale
factor when zooming in/out, clamp that factor to [minScale, maxScale], and only
call m_view->scale(...) (or adjust to an exact factor to reach the clamp
boundary) when the resulting scale stays within limits; ensure the logic lives
inside eventFilter and still returns true after handling the event.
🪄 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: aa0de64f-782f-467f-b3aa-2e7650cbdf85
📒 Files selected for processing (2)
src/mumble/widgets/ResponsiveImageDialog.cppsrc/mumble/widgets/ResponsiveImageDialog.h
Comment on lines
+59
to
+67
| if (obj == m_view->viewport() && event->type() == QEvent::Wheel) { | ||
| QWheelEvent *wheelEvent = static_cast< QWheelEvent * >(event); | ||
| // Zoom in or out. | ||
| if (wheelEvent->angleDelta().y() > 0) { | ||
| m_view->scale(1.1, 1.1); | ||
| } else if (wheelEvent->angleDelta().y() < 0) { | ||
| m_view->scale(1.0 / 1.1, 1.0 / 1.1); | ||
| } | ||
| return true; // Event handled. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/mumble/widgets/ResponsiveImageDialog.cpp"
# Ensure file exists
test -f "$file"
echo "== File: $file =="
nl -ba "$file" | sed -n '1,220p'
echo "== Zoom/scale related matches in file =="
rg -n "wheelEvent|QWheelEvent|angleDelta|scale\(|transform\(\)\.m11|m11\(|setTransformationAnchor|setTransform|clamp|min|max|zoom" "$file" || true
echo "== m_view->scale usage in widgets folder (context) =="
rg -n "m_view->scale\(" src/mumble/widgets | head -n 200 || true
echo "== Any existing min/max transform or zoom handling in repo (context) =="
rg -n "min.*scale|max.*scale|clamp.*scale|m11\(\).*scale|setTransform|fitInView|wheelEvent|QEvent::Wheel" src/mumble/widgets | head -n 200 || trueRepository: mumble-voip/mumble
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/mumble/widgets/ResponsiveImageDialog.cpp"
test -f "$file"
echo "== File: $file (lines 1-220) =="
cat -n "$file" | sed -n '1,220p'
echo "== Zoom/scale related matches in file =="
rg -n "wheelEvent|QWheelEvent|angleDelta|scale\(|transform\(\)\.m11|m11\(|setTransformationAnchor|setTransform|clamp|min|max|zoom" "$file" || true
echo "== m_view->scale usage in widgets folder =="
rg -n "m_view->scale\(" src/mumble/widgets | head -n 200 || true
echo "== Repo context for existing zoom/transform clamps (widgets folder) =="
rg -n "min.*scale|max.*scale|clamp.*scale|m11\(\).*scale|setTransform|fitInView|wheelEvent|QEvent::Wheel" src/mumble/widgets | head -n 200 || trueRepository: mumble-voip/mumble
Length of output: 5470
Clamp wheel zoom range to keep the view recoverable.
ResponsiveImageDialog::eventFilter() applies unbounded m_view->scale(…) on every wheel event (angleDelta().y() > 0 / < 0) with no min/max transform limits; repeated zooming can make the image effectively unusable until a resize/show resets via fitInView.
Proposed fix
if (obj == m_view->viewport() && event->type() == QEvent::Wheel) {
QWheelEvent *wheelEvent = static_cast< QWheelEvent * >(event);
- // Zoom in or out.
- if (wheelEvent->angleDelta().y() > 0) {
- m_view->scale(1.1, 1.1);
- } else if (wheelEvent->angleDelta().y() < 0) {
- m_view->scale(1.0 / 1.1, 1.0 / 1.1);
- }
+ const qreal currentScale = m_view->transform().m11();
+ const qreal factor = (wheelEvent->angleDelta().y() > 0) ? 1.1 : (1.0 / 1.1);
+ const qreal nextScale = currentScale * factor;
+ if (wheelEvent->angleDelta().y() != 0 && nextScale >= 0.1 && nextScale <= 10.0) {
+ m_view->scale(factor, factor);
+ }
return true; // Event handled.
}🤖 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 `@src/mumble/widgets/ResponsiveImageDialog.cpp` around lines 59 - 67, The wheel
handler in ResponsiveImageDialog::eventFilter currently applies m_view->scale(…)
unbounded on each QWheelEvent (angleDelta().y()), so repeatedly zooming can make
the view unusable; change it to read the current scale from m_view->transform()
(e.g. transform().m11()), define sensible minScale and maxScale constants,
compute the proposed new scale factor when zooming in/out, clamp that factor to
[minScale, maxScale], and only call m_view->scale(...) (or adjust to an exact
factor to reach the clamp boundary) when the resulting scale stays within
limits; ensure the logic lives inside eventFilter and still returns true after
handling the event.
Comment on lines
+68
to
+70
| } else if (obj == m_view && event->type() == QEvent::Resize) { | ||
| m_view->fitInView(m_scene->itemsBoundingRect(), Qt::KeepAspectRatio); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/mumble/widgets/ResponsiveImageDialog.cpp"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Show the relevant region around the reported lines
nl -ba "$FILE" | sed -n '1,140p'
# Also search for fitInView usage and any zoom/pan/auto-fit related flags
rg -n "fitInView|KeepAspectRatio|itemsBoundingRect|auto.?fit|zoom|pan" "$FILE" || true
# Search for any event handlers or state tracking in the same file
rg -n "QEvent::Resize|event->type\\(\\)|mouse|wheel|scroll|drag" "$FILE" || trueRepository: mumble-voip/mumble
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/mumble/widgets/ResponsiveImageDialog.cpp"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Print with line numbers using awk (nl isn't available)
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '50,110p'
echo "---- searches ----"
rg -n "fitInView|KeepAspectRatio|itemsBoundingRect|auto.?fit|zoom|pan" "$FILE" || true
rg -n "QEvent::Resize|event->type\\(|mouse|wheel|scroll|drag" "$FILE" || trueRepository: mumble-voip/mumble
Length of output: 1904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/mumble/widgets/ResponsiveImageDialog.cpp"
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,220p'
echo "---- additional searches ----"
rg -n "fitInView\\(|scale\\(|translate\\(|scroll|drag|mouse|pan|zoom|auto.?fit|manual|user" "$FILE" || trueRepository: mumble-voip/mumble
Length of output: 3640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/mumble/widgets/ResponsiveImageDialog.h"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo "---- searches ----"
rg -n "fitInView|auto.?fit|zoom|pan|scale|transform|eventFilter|resize" "$FILE" || trueRepository: mumble-voip/mumble
Length of output: 1243
Don’t reset user zoom/pan on every resize.
ResponsiveImageDialog::eventFilter unconditionally calls m_view->fitInView(...) on every QEvent::Resize, overriding the wheel zoom (m_view->scale(...)) and resetting the user’s current view/pan state:
} else if (obj == m_view && event->type() == QEvent::Resize) {
m_view->fitInView(m_scene->itemsBoundingRect(), Qt::KeepAspectRatio);
}Gate this resize “auto-fit” behind an “auto-fit enabled” flag (enabled initially / after show), and disable it after the first manual zoom/pan input.
🤖 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 `@src/mumble/widgets/ResponsiveImageDialog.cpp` around lines 68 - 70,
ResponsiveImageDialog currently calls m_view->fitInView(...) on every
QEvent::Resize in ResponsiveImageDialog::eventFilter, which resets user
zoom/pan; add a boolean member (e.g., m_autoFitEnabled) initialized true (set
true after show) and change the resize branch to call fitInView only when
m_autoFitEnabled is true; additionally, in eventFilter detect user-initiated
zoom/pan inputs (wheel events that call m_view->scale(...), mouse press/move for
panning, or gesture events) and set m_autoFitEnabled = false when those occur so
further resizes won’t override the user’s view (optionally expose a method to
re-enable auto-fit).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This improves the current image viewer originally implemented 9 months ago.
It now preserves the image aspect ratio correctly by introducing zoom support, click-and-drag navigation, and panning functionality for a more usable viewing experience.
It was tested and works correctly on my openSUSE Tumbleweed installation
Checks