saket / telephoto

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

SubcomposeAsyncImage like API #35

Closed lukaspieper closed 1 year ago

lukaspieper commented 1 year ago

Hi Saket,

Coil offers SubcomposeAsyncImage where I like to use loading, error and success for overlays. Would be create to have something similar for ZoomableImage.

saket commented 1 year ago

I originally decided this to keep the API surface small. Is it possible for you to manually observe your request's state? Something along these lines:

var isImageLoaded by remember { mutableStateOf(false) }

Box {
  ZoomableAsyncImage(
    model = ImageRequest.Builder(LocalContext.current)
      .data("https://example.com/image.jpg")
      .listener(
        onSuccess = { isImageLoaded = true },
      )
      .build(),
    contentDescription = …
  )

  if (!isImageLoaded) {
    ProgressIndicator()
  }
}
lukaspieper commented 1 year ago

Thanks for reminding me of that API. Can definitively understand that you want to keep Telephoto's API small, and I think that is a good thing. Need to rework the code then quite a bit.

Thanks for creating an sharing this library btw, something like this was long missing for Compose! Looking forward to the next release, seen you worked on EXIF.

DikenMaharjan commented 1 month ago

I only want to display the loading if there are no placeholder. But the eventListener doesn't expose any information on whether the place holder is found or not. Setting target would inform us whether there is place holder or not, but it is being replaced internally to get the underlying placeholder. Would it be possible to create a delegating Target, that would not lose the initlal target. Something like this:

class TargetDelegate(
    private val initial: coil.target.Target,
    private val error: (error: Drawable?) -> Unit = {},
    private val start: (placeholder: Drawable?) -> Unit = {},
    private val success: (result: Drawable) -> Unit = {}
) : coil.target.Target {
    override fun onError(error: Drawable?) {
        initial.onError(error)
        error(error)
    }

    override fun onStart(placeholder: Drawable?) {
        initial.onStart(placeholder)
        start(placeholder)
    }

    override fun onSuccess(result: Drawable) {
        initial.onStart(result)
        success(result)
    }
}

and setting the target in me.saket.telephoto.zoomable.coil.Resolver as

 initialRequest.newBuilder()
        .target(
            initialRequest.target?.let {
                TargetDelegate(
                    initial = it,
                    start = {
                        resolved = resolved.copy(
                            placeholder = it?.asPainter(),
                        )
                    }
                )
            } ?: object : coil.target.Target {
                override fun onStart(placeholder: Drawable?) {
                    super.onStart(placeholder)
                    resolved = resolved.copy(
                        placeholder = it?.asPainter(),
                    )
                }
            }
        ).build()
DikenMaharjan commented 1 month ago

For my usecase, I found that ZoomableImageState contains isPlaceholderDisplayed parameter. I will be using that.

Regardless, Do you think you will migrate to DelegateTarget to not lose the initial target? @saket