Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes in ZoomLayout's Width/Height don't update child transformation to keep center-crop working #186

Open
rupinderjeet opened this issue Oct 16, 2020 · 5 comments

Comments

@rupinderjeet
Copy link

rupinderjeet commented Oct 16, 2020

Describe the bug

  • Library version: 1.8.0
  • Reproducible in official demo app: Yes
  • Device / Android version: Pixel 3, API 29

I am using ZoomLayout to show portrait/landscape images. I have disabled Zoom & OverScroll. I am using horizontal & vertical Pan with CenterCrop only. At one time, only one direction pan can be performed, as expected(because zoom disabled). This allows me to show a portion of image, while the other portion can be viewed using Pan.

As I have understood, centercrop works by aligning either containerHeight to childHeight or containerWidth to childWidth. Right? Imagine loading landscape/portrait images into a square ImageView. For landscape images, childHeight will be aligned with containerHeight to perform centercrop, and now, pan can be performed horizontally. Similarly, for portrait images, pan can be performed vertically.

This works fine.

But, if I change width/height of ZoomLayout like this, the transformation is not applied on child view.

val layoutParams = zoomLayout.layoutParams
layoutParams.width = layoutParams.width + changedWidth
layoutParams.height = layoutParams.height + changedHeight
zoomLayout.layoutParams = layoutParams

To Reproduce

Steps to reproduce the behavior, possibly in the demo app:

  1. Disable Zoom and OverScroll.
  2. Enable Pan for Horizontal & Vertical.
  3. Place a child View in ZoomLayout in XML. Keep width/height to wrap content.
  4. Add a long landscape drawable. Tested with 780 x 180.
  5. Set this drawable as background for child view in ZoomLayout in XML.
  6. Run, perform Pan horizontally.
  7. Use a button to increase height of ZoomLayout. Transformation on child is not performed.

Expected behavior

When container height changes, child transformation is applied to maintain CenterCrop.

XML layout (Extended ZoomLayout)

init {
	setHorizontalPanEnabled(true)
	setVerticalPanEnabled(true)

	setOverScrollHorizontal(false)
	setOverScrollVertical(false)

	setAlignment(Alignment.TOP or Alignment.LEFT)
	setHasClickableChildren(true)
	setTransformation(
		ZoomApi.TRANSFORMATION_CENTER_CROP,
		ZoomApi.TRANSFORMATION_GRAVITY_AUTO
	)

	setZoomEnabled(false)
}

Screenshots

Screenshot

Screenshot

Logs

Launched

I/ZoomLayout: setHasClickableChildren: old: false new: false
I/ZoomLayout: setHasClickableChildren: old: false new: true

Load Horizontal Background Drawable on Child.

W/ZoomEngine: onMatrixSizeChanged: firstTime: true oldZoom: NaN transformation: 1 transformationZoom: 0.0
I/ZoomEngine: computeTransformationZoom centerCrop scaleX: 1.0042135 scaleY: 5.860656
I/ZoomEngine: onMatrixSizeChanged: newTransformationZoom: 5.860656 newRealZoom: 5.8606563 newZoom: 1.0000001

Increase ZoomLayout Height using above code.

W/ZoomEngine: onMatrixSizeChanged: firstTime: false oldZoom: 5.8606563 transformation: 1 transformationZoom: 5.860656
I/ZoomEngine: computeTransformationZoom centerCrop scaleX: 1.0042135 scaleY: 6.2704916
I/ZoomEngine: onMatrixSizeChanged: newTransformationZoom: 6.2704916 newRealZoom: 5.8606563 newZoom: 0.93464065

Child is not transformed to maintain centercrop functionality.

APK

Will provide if needed, but very easy to use reproduction steps.

@rupinderjeet rupinderjeet changed the title ZoomEngine#setContainerSize doesn't refresh child transformation when CenterCrop is enabled Changes in ZoomLayout's Width/Height don't update child transformation to keep center-crop working Oct 16, 2020
@rupinderjeet
Copy link
Author

I used below code to get around this.

val layoutParams = zoomLayout.layoutParams
layoutParams.width = layoutParams.width + changedWidth
layoutParams.height = layoutParams.height + changedHeight
zoomLayout.layoutParams = layoutParams

zoomLayout.engine.setContainerSize(
	layoutParams.width.toFloat(),
	layoutParams.height.toFloat(),
	applyTransformation = false // not applying transformation
)

And, another change in ZoomLayout's onGlobalLayout()

engine.setContentSize(
	child.width.toFloat(), 
	child.height.toFloat(),
	applyTransformation = true // <-- this change
)

Doing this helps me. Child is correctly aligned with parent to maintain centercrop functionality. But, this code births #185 issue. Clearly, now I know the reason why Pan resets(hint#global-layout).

So, I am still looking for a way to tell ZoomLayout to apply transformation on child when container width/height is changed. If I keep trying using hacks, I will only have more bugs. You have worked on this, you know better than me. Please guide me.

@markusressel
Copy link
Collaborator

Thank you for the detailed report.
I think this is a general issue of ZoomLayout not really having a defined behavior when changing the dimensions of the container yet, since we expect the container to keep its dimensions, please correct me if I'm wrong @natario1. So as I understand it this is more of a feature request than a bug.

If we want to support this, I think we need to have a discussion about how the expected behavior should look like, and how developers should be able to deviate from it.

@natario1
Copy link
Owner

I like how it works right now, container size can change for many reasons and if the content was already zoomed/panned, I don't think we should re-apply the transformation. You should be able to achieve what you want like this:

zoomLayout.layoutParams = layoutParams
zoomLayout.engine.setContainerSize(layoutParams.width, layoutParams.height, true)

Or maybe like this

zoomLayout.layoutParams = layoutParams
zoomLayout.post {
    zoomLayout.engine.setContainerSize(layoutParams.width, layoutParams.height, true)
}

We could make this easier with a syncTransformation() function or something like that

@rupinderjeet
Copy link
Author

@markusressel Okay, I understand. Also, I feel what I want to do is only using 25% potential of this library. Disabling what the library is meant to be used for is actually kind-of going against it and expecting it to work. All I need is center-crop with pan and fling. If you have anything to suggest, please do. Thanks.

container size can change for many reasons and if the content was already zoomed/panned, I don't think we should re-apply the transformation

@natario1 If this is expected/needed, there should be an opt-in flag to this behaviour. To me, it feels like anti of default expectation. When center-crop is applied, and container size is changed, center-crop should be maintained. Although if we aren't supporting size changes in ZoomLayout, then this wouldn't even matter.

Also, I am engaged with library's code from Tuesday. I do not know much anything about how matrices, vectors or shaders work. I try to read and modify to learn. But, I've already tried out your suggestion before reporting the issue. As an outcome of it, Pan is enabled for both horizontal & vertical directions. And, the center-crop is no longer available as the content/child appears to be zoomed.

Thank you for your help. I am sorry I took much longer to respond.

@markusressel
Copy link
Collaborator

markusressel commented Oct 27, 2020

@rupinderjeet I tried to reproduce your example, but when applying the modified LayoutParams to the ZoomLayout instance, I get our very own error message:

java.lang.RuntimeException: ZoomLayout must be used with fixed dimensions (e.g. match_parent)

Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants