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

2.x Fix implicit conversion from float to int #2775

Merged
merged 7 commits into from Jul 28, 2023

Conversation

gchtr
Copy link
Member

@gchtr gchtr commented Jul 20, 2023

Related:

Issue

We require dimensions to be int, but SVG might have float dimensions.

Solution

Convert to int.

Impact

No deprecation notice.

Usage Changes

None.

Considerations

Should we round the values before typecasting them to int using round()?

I think we also have to consider whether it might be important to get SVG dimensions as floats. For using the dimensions as width and height attributes in HTML, it has to be an integer value.

But maybe there’s a use case where you’ll want to access SVG dimensions as floats. In that case, we might somehow have to give access to ImageDimensions::get_dimensions_svg(), which is protected at the moment:

protected function get_dimensions_svg($svg)

Testing

No.

@Levdbas
Copy link
Member

Levdbas commented Jul 24, 2023

I think there is no need to disclose the original values via get_dimensions_svg() since having SVG widths and heights as floats are bad to begin with in my experience. I exerienced a lot of issues with for example hairlines when scaling a svg down with a large number after the decimal as width/height. And I think rounding the numbers is the way to go before typecasting them.

@Levdbas
Copy link
Member

Levdbas commented Jul 25, 2023

It seems like the tests asserts that the SVG viewbox width of 530.91 is rounded down to 530, instead of up. In my opinion the current round function is what we would like.

We could change the testing value to 531 or the actual svg viewbox width 529.91, which gets then rounded up to 530?

But correct me if I am wrong @gchtr !

Levdbas
Levdbas previously approved these changes Jul 27, 2023
@gchtr gchtr merged commit 9c344c9 into 2.x Jul 28, 2023
52 checks passed
@gchtr gchtr deleted the 2.x-implicit-conversion-from-float-to-int branch July 28, 2023 06:24
@gchtr
Copy link
Member Author

gchtr commented Jul 28, 2023

@Levdbas Thanks for the hint! I didn’t runt the tests locally, so I missed the rounded numbers issue.

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

Successfully merging this pull request may close these issues.

None yet

2 participants