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

FlickToDismiss drops frames when it is removed from composition before it completes its dismiss animation #47

Open MV-GH opened 9 months ago

MV-GH commented 9 months ago

Follow up on https://github.com/saket/telephoto/issues/43#issuecomment-1707515901

I have implemented your library to replace the existing functionality and it seems to work well.

But I was wondering if I could remove the delay of the the dismiss action?

If I remove the delay it becomes very choppy. But I would like it to be instant like the other functionality, I already have.

    (flickState.gestureState as? FlickToDismissState.GestureState.Dismissing)?.let { gestureState ->
        LaunchedEffect(Unit) {
            delay(gestureState.animationDuration / 6) // Seems to be the lowest I can get away with.
            appState.navigateUp()
        }
    }

see choppyness

https://github.com/saket/telephoto/assets/67873169/ead87f0e-f209-4715-8c39-11743abb5106

saket commented 9 months ago

This entirely depends on your navigation stack. If your app only supports showing one screen at a time then you can't do much. You must wait till the dismiss animation is complete before removing it from the UI hierarchy.

Can you refactor your media viewer screen to be shown as an overlay above other screens? This way you'll also be able to use a translucent background for media as they're being swiped up/down for dismissal.

MV-GH commented 9 months ago

We used to have it as a overlay through a Dialog but that had numerous issues. So I had resort to using a separate screen. (see https://github.com/dessalines/jerboa/pull/980)

saket commented 9 months ago

Yeah dialogs can be annoying. But does compose-navigation allow other ways of showing overlays without using a dialog? Can Popup() be used?

MV-GH commented 9 months ago

We do have some places that we use a Popup. But I am reluctant to make this change as we have dropped Flick for now (and the previous impl). As Flicks behaviour doesn't really fit for our usecase. As soon it enters the flick animation due to moving touch up or down. It will no longer allow to zoom. While ideally we want when a movement happens above a certain threshold. It would dismiss the action without having a impact on any actions listening to that movement.

saket commented 9 months ago

As soon it enters the flick animation due to moving touch up or down. It will no longer allow to zoom. While ideally we want when a movement happens above a certain threshold

Do I understand this correctly that FlickToDismiss is too sensitive right now?

MV-GH commented 9 months ago

You could say that but what they want, is to not be interrupted while attempting to zoom. (I am mostly assuming as I don't really experience this, maybe one finger comes down significantly before the other, or their devices are much more sensitive)

Feedback i got:

It’s pretty much back to the behavior explained above. If you slightly move your fingers before zooming in, you are stuck at flicking the image away and can’t zoom.

It’s not, as soon as the image moves vertically I can’t zoom in anymore and it is easy to accidentally make it move before pinching. I think the image shouldn’t respond to vertical swipes with two fingers at all and should have a small dead zone for single finger swipes. That’s how WhatsApp seems to do it Also the black background takes too long to disappear after a flick, it could be nicer if it faded away