Set minimum pinch distance to 44#159
Conversation
|
@claude please review this PR |
|
@claude review |
…ch-distance-44 # Conflicts: # lib/commonjs/ReactNativeZoomableView.js # lib/commonjs/ReactNativeZoomableView.js.map # lib/commonjs/animations/index.js # lib/commonjs/animations/index.js.map # lib/commonjs/components/AnimatedTouchFeedback.js # lib/commonjs/components/AnimatedTouchFeedback.js.map # lib/commonjs/components/StaticPin.js # lib/commonjs/components/StaticPin.js.map # lib/commonjs/components/index.js # lib/commonjs/components/index.js.map # lib/commonjs/debugHelper/index.js # lib/commonjs/debugHelper/index.js.map # lib/commonjs/helper/applyPanBoundariesToOffset.js # lib/commonjs/helper/applyPanBoundariesToOffset.js.map # lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js # lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js.map # lib/commonjs/helper/coordinateConversion.js # lib/commonjs/helper/coordinateConversion.js.map # lib/commonjs/helper/index.js # lib/commonjs/helper/index.js.map # lib/commonjs/index.js # lib/commonjs/index.js.map # lib/commonjs/typings/index.js # lib/commonjs/typings/index.js.map # lib/module/ReactNativeZoomableView.js # lib/module/ReactNativeZoomableView.js.map # lib/module/animations/index.js # lib/module/animations/index.js.map # lib/module/components/AnimatedTouchFeedback.js # lib/module/components/AnimatedTouchFeedback.js.map # lib/module/components/StaticPin.js # lib/module/components/StaticPin.js.map # lib/module/components/index.js # lib/module/components/index.js.map # lib/module/debugHelper/index.js # lib/module/debugHelper/index.js.map # lib/module/helper/applyPanBoundariesToOffset.js # lib/module/helper/applyPanBoundariesToOffset.js.map # lib/module/helper/calcNewScaledOffsetForZoomCentering.js # lib/module/helper/calcNewScaledOffsetForZoomCentering.js.map # lib/module/helper/coordinateConversion.js # lib/module/helper/coordinateConversion.js.map # lib/module/helper/index.js # lib/module/helper/index.js.map # lib/module/index.js # lib/module/index.js.map # lib/module/typings/index.js # lib/module/typings/index.js.map # lib/typescript/ReactNativeZoomableView.d.ts # lib/typescript/animations/index.d.ts # lib/typescript/components/AnimatedTouchFeedback.d.ts # lib/typescript/components/StaticPin.d.ts # lib/typescript/components/index.d.ts # lib/typescript/debugHelper/index.d.ts # lib/typescript/helper/applyPanBoundariesToOffset.d.ts # lib/typescript/helper/calcNewScaledOffsetForZoomCentering.d.ts # lib/typescript/helper/coordinateConversion.d.ts # lib/typescript/helper/index.d.ts # lib/typescript/index.d.ts # lib/typescript/typings/index.d.ts
PR #158 removed checked-in lib/ and added it to .gitignore. These remaining tracked files are now redundant.
There was a problem hiding this comment.
The fix described in your reply (returning null instead of clamping) hasn't been committed to the branch — the file still returns Math.max(MIN_PINCH_DISTANCE, Math.sqrt(dx * dx + dy * dy)), which has the zoom-out artifact identified in the prior review.
Extended reasoning...
Overview
The PR modifies src/helper/index.ts to add a MIN_PINCH_DISTANCE = 44 constant and applies it via Math.max clamping in calcGestureTouchDistance.
Critical unresolved bug
The prior review flagged a red-severity bug: clamping to 44 rather than returning null causes a ratio mis-calculation on the first frame when fingers cross below the threshold, producing a visible zoom-out artifact. The author replied that this was fixed by returning null, but the actual file (src/helper/index.ts line 58) still contains return Math.max(MIN_PINCH_DISTANCE, Math.sqrt(dx * dx + dy * dy));. No new commit reflecting the described fix is visible in the branch history.
Security risks
None — this is pure gesture math with no auth, data, or IO concerns.
Level of scrutiny
The change touches the core pinch-zoom calculation path used on every pinch gesture, so correctness matters. The fix is simple once the right approach (return null) is known, but the branch needs to actually contain it.
Other factors
The only new bug in this run is a nit about the HIG URL fragment (the constant value is correct). That alone would not block approval. The blocker is the red bug from the prior review that the author described fixing but did not commit.
| const dy = Math.abs(touches[0].pageY - touches[1].pageY); | ||
| return Math.sqrt(dx * dx + dy * dy); | ||
|
|
||
| return Math.max(MIN_PINCH_DISTANCE, Math.sqrt(dx * dx + dy * dy)); |
There was a problem hiding this comment.
is there any issue if the pinch distance isn't based on zoom?
There was a problem hiding this comment.
I'm not sure what you mean by this?
There was a problem hiding this comment.
ah nvm these are screen coordinates so not dictated by current zoom scale
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…acelabs/react-native-zoomable-view into elliott/minimum-touch-distance-44
| const actualDistance = Math.sqrt(dx * dx + dy * dy); | ||
| if (actualDistance < MIN_PINCH_DISTANCE) return null; | ||
| return actualDistance; |
There was a problem hiding this comment.
🟡 This check feels out of place in the helper. calcGestureTouchDistance advertises one job in its name and signature: return the Euclidean distance between two touches. Returning null for "valid pinch geometry not met" overloads it with caller policy — null already means "not 2 fingers" per the existing guard at line 52, so a second meaning ("2 fingers but too close") makes the contract ambiguous at every call site (_handlePinching early-returns, the two reseed sites at onPanResponderMove would silently skip seeding lastGestureTouchDistance, etc.).
Suggested shape — keep the helper pure and move both the constant and the gate into ReactNativeZoomableView.tsx next to the other gesture sentinels:
// src/helper/index.ts — back to pure math
export function calcGestureTouchDistance(
e: GestureResponderEvent,
gestureState: PanResponderGestureState
): number | null {
const touches = e.nativeEvent.touches;
if (gestureState.numberActiveTouches !== 2 || !touches[0] || !touches[1])
return null;
const dx = Math.abs(touches[0].pageX - touches[1].pageX);
const dy = Math.abs(touches[0].pageY - touches[1].pageY);
return Math.sqrt(dx * dx + dy * dy);
}// src/ReactNativeZoomableView.tsx — gate at the gesture state machine
const MIN_PINCH_DISTANCE = 44;
// inside _handlePinching, replacing the existing distance / anchor gates:
const distance = calcGestureTouchDistance(e, gestureState);
if (!distance) return;
if (!lastGestureTouchDistance.current) {
// Anchor lazily once fingers are far enough apart for the math to be stable.
if (distance >= MIN_PINCH_DISTANCE) lastGestureTouchDistance.current = distance;
return;
}
// existing growth / zoom math unchangedSame end behavior for the case the PR targets, with three benefits:
- The helper keeps a single meaning.
nullonly ever signals "not a 2-finger event." - The threshold is checked once at pinch start (anchor moment), not on every frame, so a pinch already in flight doesn't drop frames if the fingers momentarily come closer than 44pt — relevant at higher zoom levels where users hold fingers closer.
- The policy lives next to the gesture state it gates, where it's discoverable when someone next touches the pinch logic.
There was a problem hiding this comment.
The threshold is checked once at pinch start (anchor moment), not on every frame, so a pinch already in flight doesn't drop frames if the fingers momentarily come closer than 44pt
I think this was where the problem was - fingers getting close during pinch was making things go crazy
— relevant at higher zoom levels where users hold fingers closer.
Does the zoom level affect punch distance? Isn't this screen distance??
Below a certain number of pixels, the math for pinching stops being accurate and the scale starts to do strange things. This PR introduces a minimum distance of 44pt between the fingers for a pinch gesture.
Note
react/jsx-runtime, adopts class fields and optional chaining, and tidies event-callback wiring without changing public APIs.AnimatedTouchFeedback,StaticPin, and animation helpers to the new runtime and minor refactors; no functional UI changes aside from the pinch threshold.Written by Cursor Bugbot for commit 0272ef6. Configure here.