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

Fix issue with scale height 65536 used as "flexible height" #144

Merged
merged 11 commits into from
Sep 26, 2023

Conversation

petschki
Copy link
Member

@petschki petschki commented Sep 25, 2023

In Plone 6 there are image scales with height 65536 in order to make the height calculated flexible when scaling down to the width. This is a problem in cropping editor, because there the flag can_scale checks if width and height of scaled image is larger than the original image.

To make these scales croppable here's my solution proposal:

  • can_scale only checks that the width of the scaled image is lower equal to the original width.
  • if the scaled image height is 65536 we calculate the height for the editor by the ratio of the original width/height.

I'm not really happy with the hard coded pixel value though. I also thought about a checkbox in the settings to discard the maximum check for the editor and leave it up to the integrator to constrain the scales to the valid one.

feedback welcome...

see #130

@MrTango
Copy link
Contributor

MrTango commented Sep 25, 2023

I added a test to verify the desired behavior.
Currently the last assert fails, because we allow a height higher than original image, even if it is under the max value of 65536.
That does not make sense.

@MrTango
Copy link
Contributor

MrTango commented Sep 25, 2023

but also the test_create_new_hashkey_for_cropped_images is still failing and i think not without reason.

@petschki petschki marked this pull request as ready for review September 26, 2023 13:16
@petschki petschki merged commit 97b4086 into master Sep 26, 2023
7 checks passed
@petschki petschki deleted the petschki-imagescales branch September 26, 2023 13:48
@laulaz
Copy link
Member

laulaz commented Sep 26, 2023

🙏

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

Successfully merging this pull request may close these issues.

3 participants