saket / telephoto

Building blocks for designing media experiences in Compose UI
https://saket.github.io/telephoto/
Apache License 2.0
869 stars 28 forks source link

Double tap not working sometimes #43

Closed igorilin13 closed 10 months ago

igorilin13 commented 10 months ago

Hi there, I noticed sometimes a double tap gesture is interpreted as something else which results in a strange looking behavior where you double tap and the zoom changes by a tiny bit (like 0.5-1%). This is pretty easy to reproduce, at least on my device. I can record a video if that would help.

It seems to be caused by this part in PointerInputScope.detectTapAndQuickZoomGestures:

var dragged = false
verticalDrag(secondDown.id) { drag ->
  dragged = true
  val dragDelta = drag.positionChange()
  val zoomDelta = 1f + (dragDelta.y * 0.004f) // Formula copied from https://github.com/usuiat/Zoomable.
  onQuickZoom(Zooming(secondDown.position, zoomDelta))
  drag.consume()
}

if (dragged) {
  onQuickZoom(QuickZoomStopped)
} else {
  onDoubleTap(secondDown.position)
}

It calls onQuickZoom instead of onDoubleTap. Could you provide some info on why onQuickZoom() is there and what kind of gesture it covers? Thanks

saket commented 10 months ago

I noticed sometimes a double tap gesture is interpreted as something else which results in a strange looking behavior where you double tap and the zoom changes by a tiny bit (like 0.5-1%).

Can you share a video? I've a theory on why this might be happening but I'd like to verify it.

igorilin13 commented 10 months ago

I noticed sometimes a double tap gesture is interpreted as something else which results in a strange looking behavior where you double tap and the zoom changes by a tiny bit (like 0.5-1%).

Can you share a video? I've a theory on why this might be happening but I'd like to verify it.

https://github.com/saket/telephoto/assets/24581068/97ec70c9-63df-4e04-8717-59d398d79001

saket commented 10 months ago

Huh I am not sure what's happening there. Any chances you could help me investigate by adding breakpoints/print statements to ZoomableState? There are two functions that can interrupt a double-tap-to-zoom gesture:

  1. refreshContentTransformation(). This gets called if a change is detected in any of the layout parameters such as its size, alignment, etc.

https://github.com/saket/telephoto/blob/ab5fdaf5c2ccb2468611842785e5d5b76ec32618/zoomable/src/commonMain/kotlin/me/saket/telephoto/zoomable/ZoomableState.kt#L396-L398

  1. resetZoom(). This can only be called by the app.

https://github.com/saket/telephoto/blob/ab5fdaf5c2ccb2468611842785e5d5b76ec32618/zoomable/src/commonMain/kotlin/me/saket/telephoto/zoomable/ZoomableState.kt#L384-L386

saket commented 10 months ago

Ohey I was able to reproduce this on a Samsung Galaxy A20. It seems like compose is detecting very tiny drags on phones with non-stellar displays. I'll have to ignore quick zooms until the drag distance crosses a certain threshold.

MV-GH commented 10 months ago

I was about to make a report about something like that. My users report something that I can't reproduce but is not about double tap. But about zooming not always working reliably.

Pinching the screen (especially with single hand) doesn’t reliably zoom images and I have to try multiple times to get it to work

Which can reproduce by doing (according to them)

If you put 2 fingers on the screen in a fullscreen image and slide them even the tiniest millimeter it will go into “pan mode” or so instead of zooming mode. You can no longer zoom until you release your fingers and place them without moving them at all before you pinch to zoom. (I have this happen much more often in practice while using the phone 1 handed on an unstable environment like a tram or train.

Interestingly, you can pan and zoom at the same time if already zoomed in, but on a full image where panning doesn’t do anything, it will lock both actions until you release your fingers.

Do you think this was caused by the same issue or was this something else?

see context https://programming.dev/post/2375491

saket commented 10 months ago

I believe that's happening because your vertical scrollable modifier is interfering with telephoto's gesture detector. Can it be removed? Compose does not support 2d nested scrolling so unfortunately they can't play nice with each other automatically.

https://github.com/dessalines/jerboa/blob/5cbb7c1e43ba264847f5dc100cd83d4bbdf4db84/app/src/main/java/com/jerboa/ui/components/imageviewer/ImageViewerActivity.kt#L153-L161

MV-GH commented 10 months ago

Hmm, is there way I can keep the swipe up to exit feature while removing the nested scrolling? I'll try make a build without it and ask if they can still reproduce it

saket commented 9 months ago

Wanna try out telephoto's upcoming FlickToDismiss() composable? I'm still ironing out the edges and the APIs might change before the public release, but it's very much usable.

implementation "me.saket.telephoto:flick:0.6.0-SNAPSHOT"

Usage:

val flickState = rememberFlickToDismissState()
FlickToDismiss(flickState) {
  ZoomableAsyncImage(…)
}

(flickState.gestureState as? Dismissing)?.let { gestureState ->
  LaunchedEffect(Unit) {
    delay(gestureState.animationDuration / 2)
    navigator.goBack()
  }
}
MV-GH commented 9 months ago

I got back from one of my users that the problem did seem to be resolved by removing the nested scroll. Once I have some time I'll take a look at ur library and see if I can implement it. Thanks for investigating!