saket / telephoto

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

Zoomable modifier looses state after ZoomableState gets disposed and recreated #72

Closed CalamityDeadshot closed 8 months ago

CalamityDeadshot commented 8 months ago

I'm using Modifier.zoomable inside a HorizontalPager - a pretty standard use case.
My issue is that after a page is composed for a second time RealZoomableState looses its gestureState synchronization for some reason, so when ZoomableNode's onTransformStopped calls state.isZoomOutsideRange() its val userZoomFactor = gestureState?.userZoomFactor pretty much equals 1 for me, so state.smoothlySettleZoomOnGestureEnd() is not called whatsoever, and no zoom bounds are enforced.
A minimal repro with coil:

HorizontalPager(
    modifier = Modifier.fillMaxSize(),
    state = rememberPagerState(
        pageCount = { contents.size }
    ),
    beyondBoundsPageCount = 1
) { page ->
    val content = contents[page]
    val painter = rememberAsyncImagePainter(content.url)
    val zoomableState = rememberZoomableState(
        zoomSpec = ZoomSpec(
            maxZoomFactor = 3f,
            preventOverOrUnderZoom = false
        )
    )
    LaunchedEffect(painter.intrinsicSize) {
        if (painter.intrinsicSize.isSpecified) {
            zoomableState.setContentLocation(
                ZoomableContentLocation.scaledToFitAndCenterAligned(painter.intrinsicSize)
            )
        }
    }
    val contentModifier = Modifier
        .fillMaxSize()
        .zoomable(
            state = zoomableState
        )
    Image(
        painter = painter,
        contentDescription = null,
        contentScale = ContentScale.Fit,
        modifier = contentModifier,
    )
}

Here's how it looks for users:

https://github.com/saket/telephoto/assets/44675043/a4c1978f-924c-4ab2-b244-633fe01fd122

The same thing happens with pages that are composed for the first time if I scroll enough pages forwards/backwards from initialPage. Is my usage incorrect in some way?

Versions:

    kotlinCompilerExtensionVersion = "1.5.1"

    implementation(platform("androidx.compose:compose-bom:2024.02.00")) // So, androidx.compose.foundation:foundation is 1.6.2
    implementation("io.coil-kt:coil-compose:2.1.0")
    implementation("me.saket.telephoto:zoomable:0.8.0")
CalamityDeadshot commented 8 months ago

Also, flings no longer work in this situation either

saket commented 8 months ago

Is this no longer an issue?

CalamityDeadshot commented 8 months ago

It is. I honestly don't know why it's closed. I don't remember closing it. Maybe a missclick

CalamityDeadshot commented 8 months ago

Ah, I see. I referenced this issue in a commit to a private repo with a message containing Still need to fix https://github.com/saket/telephoto/issues/72. I guess GitHub tried to be clever. Funny

kvn-stgl commented 8 months ago

I think I have the same problem with the SubSamplingImage (with the ZoomableAsyncImage everything works as expected, sadly I can't use it for reasons).

I've changed the sample app a bit to reproduce the issue: https://github.com/kvn-stgl/telephoto/commit/b1b74ac5b57f7345ee7cedf0ec3382a68b8af7bb You only have to swipe the pager forward and backward. After the backward swipe, the double tab and back fling do not work anymore. Only the multitouch zoom.

Here is also a short video:

https://github.com/saket/telephoto/assets/159536/0d629147-ec7e-4730-be1c-b13d469d815f

saket commented 8 months ago

@kvn-stgl I was able to reproduce the problem using your sample, thank you! This is related to https://github.com/saket/telephoto/issues/70.

@kvn-stgl and @CalamityDeadshot I'm also curious to learn your reasons for not using ZoomableAsyncImage(). Are both of you writing a multiplatform app?

saket commented 8 months ago

1749177719156efe8ed5a7ce8fa15b7f54778f8c was deployed to maven. Can you please try out 0.9.0-SNAPSHOT?

CalamityDeadshot commented 8 months ago

reasons for not using ZoomableAsyncImage()

My gallery displays multiple types of content, like photos and videos, so it just makes more sense to use Modifier.zoomable for different types of content rather than implement a different solution for each one.

CalamityDeadshot commented 8 months ago

Can you please try out 0.9.0-SNAPSHOT?

I no longer experience this issue with me.saket.telephoto:zoomable:0.9.0-SNAPSHOT. Thank you for a quick fix!

saket commented 8 months ago

@CalamityDeadshot glad to hear!

My gallery displays multiple types of content, like photos and videos, so it just makes more sense to use Modifier.zoomable for different types of content rather than implement a different solution for each one.

This is exactly the usecase I wanted to support with having a decoupled ZoomableState. You shouldn't need to touch the lower level APIs for sharing your zoomable code between images and videos:

val zoomableState = rememberZoomableState()

when (media) {
 is Image -> {
    ZoomableAsyncImage(
     model = media.imageUrl,
     state = rememberZoomableImageState(zoomableState),
    )
  }
  is Video -> {
    ZoomableVideoPlayer(
      model = media.videoUrl,
      state = rememberZoomableExoState(zoomableState),
    )
  }
}

I'm going ahead and closing this issue, but let's continue this discussion because I want to nudge developers away from maintaining their own implementation of ZoomableAsyncImage().

CalamityDeadshot commented 8 months ago

You shouldn't need to touch the lower level APIs for sharing your zoomable code between images and videos

It's more of a matter of interoperability for me, as my gallery uses Painters as models for images and generic composable content models for videos.

val contentModifier = Modifier
    .fillMaxSize()
    .zoomable(
        state = zoomableState,
        onClick = { onClick() }
    )
when (model) {
    is Painter -> {
        // ...
        Image(
            painter = model,
            contentDescription = null,
            contentScale = ContentScale.Fit,
            modifier = contentModifier,
        )
    }
    is ComposeModel -> {
        // ...
        Box(
            modifier = contentModifier,
            contentAlignment = Alignment.Center
        ) {
            model.content()
        }
    }
}

This comes from a fork of this gallery which had its own transform gestures implementation, but it broke horribly (with a fatal crash) after I updated androidx.compose.foundation:foundation version to gain access to essential new features. It happened at such an unfortunate time, too - release deadline is around the corner, and I could not spare time to find the bug and fix it (this lib's code is, um... hard to reason about). I'm lucky I found Telephoto :)
So, basically, you are probably right, but I don't have the resources to refactor this at the moment, as this gallery API is currently used all over the app, - but I most certainly intend to! Sub-sampling alone is a very cool feature.

saket commented 8 months ago

Got it. Thanks for explaining!