saket / telephoto

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

Expose `RelativeContentLocation` as `ZoomableContentLocation.relative` #34

Closed AntsyLich closed 10 months ago

AntsyLich commented 1 year ago

In our use case our Image has a ContentScale.Fit using that with either ZoomableContentLocation.scaledInsideAndCenterAligned(canvasSize) or ZoomableContentLocation.unscaledAndTopStartAligned(canvasSize) in ZoomableState.setContentLocation gave a weirdly scaled image in case the image is smaller than screen resolution.

Copying the RelativeContentLocation from the source code and using RelativeContentLocation(canvasSize, ContentScale.Fit, Alignment.Center) gave me the same result i would expect from using ContentScale.Fit. This leads me to believe that it is worth to expose a ZoomableContentLocation.relative companion function that passes the variables to RelativeContentLocation and returns it back as ZoomableContentLocation

If accepted I can make a PR

saket commented 1 year ago

… gave a weirdly scaled image in case the image is smaller than screen resolution.

Interesting. ZoomableImage uses scaledInsideAndCenterAligned to support this same use case -- images that are smaller than layout bounds. Can you share some code, the images you're using and your device resolution?

AntsyLich commented 1 year ago

In case of ZoomableImage the underline Image's content scale is ContentScale.Inside using ContentScale.Inside with scaledInsideAndCenterAligned doesn't produce any issue but in my case i want to use ContentScale.Fit which with scaledInsideAndCenterAligned and an image smaller than screen resolution makes the displayed image weirdly zoomed in. In my case I tested with the small image below on a 720x1600 resolution device

Untitled

AntsyLich commented 1 year ago

@saket any update?

saket commented 1 year ago

Sorry, not yet. I'm traveling right now, but can take a look when I'm back in 2 weeks.

On Fri, Jun 30, 2023, 4:28 AM AntsyLich @.***> wrote:

@saket https://github.com/saket any update?

— Reply to this email directly, view it on GitHub https://github.com/saket/telephoto/issues/34#issuecomment-1614313847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASG5YB7VJIINWFSXA7M7Y3XN2EZRANCNFSM6AAAAAAZSPMF7U . You are receiving this because you were mentioned.Message ID: @.***>

saket commented 1 year ago

I tried out the above image on a 720x1600 display but I'm unable to reproduce the problem. @AntsyLich please share a screenshot and some code? A reproducer project would be the best if you've got time.

ZoomableAsyncImage(
  model = R.drawable.check_pattern,
  contentDescription = …,
  contentScale = ContentScale.Fit,
)

Screenshot_1689732925

AntsyLich commented 1 year ago

I also used.

var canvasSize by remember { mutableStateOf(Size.Zero) }
LaunchedEffect(canvasSize) {
    if (canvasSize == Size.Zero) return@LaunchedEffect
    zoomState.setContentLocation(
        ZoomableContentLocation.scaledInsideAndCenterAligned(),
    )
}

If you still can't reproduce I'll get a reproducible code up

saket commented 1 year ago

If you still can't reproduce I'll get a reproducible code up

Yes please! The code snippet you shared is incomplete. It doesn't show how canvasSize is populated.

I'm also curious to know why you aren't using ZoomableImage / ZoomableAsyncImage directly? Is that because they are incorrectly scaling your image?

saket commented 10 months ago

FWIW I've added ZoomableContentLocation.scaledToFitAndCenterAligned(), but you shouldn't need to use it if you're using ZoomableAsyncImage() / ZoomableGlideImage().

https://github.com/saket/telephoto/blob/d13b51032393949ed768fa6f4251c277d91b2ad3/zoomable/src/commonMain/kotlin/me/saket/telephoto/zoomable/ZoomableContentLocation.kt#L54-L64

It'll be available in the next release.