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

Add support for no width and no height on images #715

Merged
merged 3 commits into from
Feb 11, 2024

Conversation

LaurenzV
Copy link
Contributor

No description provided.

@LaurenzV
Copy link
Contributor Author

Closes #714

@LaurenzV
Copy link
Contributor Author

The images are linked by relative paths because that's how it was before and I didn't know if I should change it. Should I embed them as base64 strings instead?

@RazrFalcon
Copy link
Owner

RazrFalcon commented Feb 11, 2024

Relative paths are fine.

As for the code, I would suggest using tiny_skia_path::Size::scale_to_width and scale_to_height. They are basically the same, but might make the code nicer. The current implementation is too verbose.
I would also suggest calling convert_user_length first and then matching Option<T> pair.

@LaurenzV
Copy link
Contributor Author

As for the code, I would suggest using tiny_skia_path::Size::scale_to_width and scale_to_height.

Seems like this API is only available for IntSize and not Size?

@RazrFalcon
Copy link
Owner

Much better now.

Seems like this API is only available for IntSize and not Size?

Weird, ok.

Also about the PR name. We do support optional width/height. We do not support aspect preserving.
So I guess it should be called: "Preserve image aspect ratio when width or height are missing.". Does this sound ok?

@RazrFalcon
Copy link
Owner

Also, I think we need tests with non-rectangular images. Otherwise the aspect is always 1:1 and we're not testing anything. No?

@RazrFalcon
Copy link
Owner

I guess current tests are fine, because you do scale the image, but the aspect is always 1:1 right now. So I guess a single test with a non-rectangular image would be nice.

Let's use this image:

image32x16

@LaurenzV
Copy link
Contributor Author

Also, I think we need tests with non-rectangular images. Otherwise the aspect is always 1:1 and we're not testing anything. No?

It's not necessarily about the aspect.

Before: If we have an image with size 100x100, but set only the height to 200, then the image would be assumed to be 100x200 -> It will get shifted instead.

Now: If we have an image with size 100x100, but set only the height to 200, then the width will be increased by the same ratio that the height would increase/decrease, so in this case 200 -> the result is 200x200.

@LaurenzV
Copy link
Contributor Author

I guess current tests are fine, because you do scale the image, but the aspect is always 1:1 right now. So I guess a single test with a non-rectangular image would be nice.

Okay, will add it.

@RazrFalcon RazrFalcon merged commit 327b076 into RazrFalcon:master Feb 11, 2024
3 checks passed
@RazrFalcon
Copy link
Owner

Thanks!

@LaurenzV LaurenzV deleted the image-fix branch February 11, 2024 15:11
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.

None yet

2 participants