Add onTap callback to ZoomablePlugin (#863)#940
Merged
Conversation
When ZoomablePlugin is enabled, its double-tap detector consumes the pointer down on every tap, so a parent Modifier.clickable never fires. That is inherent to detectTapGestures (it consumes the down regardless of whether the tap becomes a double tap), so a tap cannot both zoom the image and bubble to a parent. Expose an onTap callback so taps can be handled at the plugin level instead of relying on the parent: - Add ZoomablePlugin.onTap and thread it through ZoomableContent (android/skia), SubSamplingImage, and zoomGestures. - Install the tap detector when double-tap zoom OR onTap is set, and make onDoubleTap conditional on enableDoubleTapZoom. When both are off no tap detector is installed, so a single tap stays unconsumed and reaches a parent gesture handler (previously the only workaround). - Fix a stale KDoc example that referenced a nonexistent ZoomablePlugin(config = ...) parameter. Adds desktop runComposeUiTest coverage: double-tap still zooms and resets (regression), single tap invokes onTap, onTap fires while the parent clickable stays silent, and the tap-handling-disabled path still reaches the parent.
WalkthroughThis PR adds an optional ChangesonTap callback feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ZoomGestureDetector
participant ZoomableState
participant ZoomablePlugin
participant ParentClickable
User->>ZoomGestureDetector: single tap
alt onTap provided or double-tap zoom enabled
ZoomGestureDetector->>ZoomGestureDetector: detectTapGestures consumes pointer down
ZoomGestureDetector->>ZoomablePlugin: invoke onTap(position)
Note over ParentClickable: does not receive tap
else no tap handling configured
ZoomGestureDetector-->>ParentClickable: pointer event propagates
ParentClickable->>ParentClickable: handle click
end
User->>ZoomGestureDetector: double tap
ZoomGestureDetector->>ZoomableState: toggle zoom / reset
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
landscapist-zoomable/src/desktopTest/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomGesturesTest.kt (1)
110-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a default-config coverage case.
Tests cover onTap-enabled (consumes tap, parent doesn't fire) and tap-handling-fully-disabled (parent fires) paths well. Missing is the original issue
#863scenario with default config (enableDoubleTapZoom=true, noonTap) verifying the tap is still consumed by the detector without crashing and without reaching a parent clickable — this is the exact bug the PR references.🤖 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 `@landscapist-zoomable/src/desktopTest/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomGesturesTest.kt` around lines 110 - 169, Add a test in ZoomGesturesTest covering the default ZoomableConfig path with no onTap, since this is the `#863` scenario. Reuse zoomGestures, rememberZoomableState, and the parent clickable setup to verify a single tap is still consumed by the gesture detector, does not crash, and does not increment the parent click count. Place it alongside singleTap_invokesOnTap_notParentClickable and singleTap_reachesParent_whenTapHandlingDisabled for clear coverage of the three tap-handling modes.
🤖 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
`@landscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/ZoomablePlugin.kt`:
- Around line 63-65: The tap-consumption note in ZoomablePlugin is too broad
because zoomGestures now bypasses the tap detector when both enableDoubleTapZoom
and onTap are disabled. Update the KDoc around ZoomablePlugin and the related
wording near zoomGestures to say parent Modifier.clickable is blocked only while
tap handling is active, and keep the guidance aligned with onTap being the
replacement for single-tap handling.
---
Nitpick comments:
In
`@landscapist-zoomable/src/desktopTest/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomGesturesTest.kt`:
- Around line 110-169: Add a test in ZoomGesturesTest covering the default
ZoomableConfig path with no onTap, since this is the `#863` scenario. Reuse
zoomGestures, rememberZoomableState, and the parent clickable setup to verify a
single tap is still consumed by the gesture detector, does not crash, and does
not increment the parent click count. Place it alongside
singleTap_invokesOnTap_notParentClickable and
singleTap_reachesParent_whenTapHandlingDisabled for clear coverage of the three
tap-handling modes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41bf825d-84f5-4468-8f6d-dbe19f461993
📒 Files selected for processing (9)
gradle/libs.versions.tomllandscapist-zoomable/build.gradle.ktslandscapist-zoomable/src/androidMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.android.ktlandscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/ZoomablePlugin.ktlandscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomGestureDetector.ktlandscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.ktlandscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/subsampling/SubSamplingImage.ktlandscapist-zoomable/src/desktopTest/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomGesturesTest.ktlandscapist-zoomable/src/skiaMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.skia.kt
Comment on lines
+63
to
+65
| * **Handling taps:** because the zoom gesture detector consumes the pointer down, a parent | ||
| * `Modifier.clickable` will not receive taps while zoom gestures are enabled. Use [onTap] to react | ||
| * to single taps instead: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the tap-consumption wording.
Line 64 and Line 93 say parent clickable is blocked whenever zoom gestures are enabled, but zoomGestures now skips the tap detector when both enableDoubleTapZoom and onTap are disabled. Please document this as “while tap handling is active” to avoid contradicting the pass-through behavior.
📝 Proposed wording adjustment
- * **Handling taps:** because the zoom gesture detector consumes the pointer down, a parent
- * `Modifier.clickable` will not receive taps while zoom gestures are enabled. Use [onTap] to react
- * to single taps instead:
+ * **Handling taps:** when tap handling is active (for example, [onTap] is set or
+ * double-tap zoom is enabled), the zoom gesture detector consumes the pointer down. A parent
+ * `Modifier.clickable` will not receive those taps. Use [onTap] to react to single taps instead:
...
- * on the image, since the zoom gesture detector consumes the pointer down and a parent
- * `Modifier.clickable` therefore does not receive the tap while zoom gestures are enabled.
+ * on the image while tap handling is active, since the zoom gesture detector consumes the
+ * pointer down and a parent `Modifier.clickable` therefore does not receive the tap.Also applies to: 91-93
🤖 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
`@landscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/ZoomablePlugin.kt`
around lines 63 - 65, The tap-consumption note in ZoomablePlugin is too broad
because zoomGestures now bypasses the tap detector when both enableDoubleTapZoom
and onTap are disabled. Update the KDoc around ZoomablePlugin and the related
wording near zoomGestures to say parent Modifier.clickable is blocked only while
tap handling is active, and keep the guidance aligned with onTap being the
replacement for single-tap handling.
|
@skydoves Hello bro, sorry for the disturbance. coderabbitai is this free tool? |
Owner
Author
|
Hey @professorDeveloper, yes, it's completely free for open-source projects :) |
Oh thanks, bro. I'm a really big fan of you 🔥 |
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
2 participants
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.
Summary
Fixes #863. When
ZoomablePluginis enabled, its double-tap detector consumes the pointerdownon every tap, so a parentModifier.clickablenever fires. This is inherent todetectTapGestures(it callsdown.consume()as soon as a pointer goes down, regardless of whether the gesture becomes a double tap), and the parent'sclickablerequires an unconsumed down. A single tap therefore cannot both drive zoom and bubble to a parent.This PR exposes an
onTapcallback so taps can be handled at the plugin level, which is what the issue asks for ("provide a click callback within the ZoomablePlugin").Changes
ZoomablePlugin.onTap: ((Offset) -> Unit)?and thread it throughZoomableContent(android/skia),SubSamplingImage, andzoomGestures.onTapis set, and makeonDoubleTapconditional onenableDoubleTapZoom. When both are off, no tap detector is installed, so a single tap stays unconsumed and reaches a parent gesture handler (previously the only workaround).ZoomablePlugin(config = ...)parameter; configuration is supplied viarememberZoomableState(config = ...).The change is source-compatible:
onTapdefaults tonull, and with it unset behavior is identical to before.Usage
Tests
Adds desktop
runComposeUiTestcoverage inZoomGesturesTest(runs on the JVM viadesktopTest, no emulator):doubleTap_zoomsIn_withDefaultConfiganddoubleTap_whenZoomed_resetsZoom— regression: existing double-tap zoom/reset still works.singleTap_invokesOnTapCallback— new: a single tap firesonTap.singleTap_invokesOnTap_notParentClickable— new:onTapfires while the parent clickable stays silent (documents the intended replacement).singleTap_reachesParent_whenTapHandlingDisabled— regression: with tap handling off, a single tap still bubbles to the parent.All 5 pass locally. All targets (android/desktop/ios/macos/wasm) compile.
Notes
org.jetbrains.compose.ui:ui-test-junit4and the hostskiko-awt-runtime(classifier detected per host, so CI/Linux works) asdesktopTest-only dependencies.landscapist-core.Summary by CodeRabbit