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 #141 "Loader::raster_bounds" to work as intended. #142

Merged
merged 2 commits into from Apr 23, 2020

Conversation

@ikatson
Copy link
Contributor

ikatson commented Apr 17, 2020

This fixes issue #141, so that Loader::raster_bounds actually returns tight raster bounds.
Previously, it returned distorted bounds due to usage of "RectF::set_origin_y"
which does not preserve Rect size.

This fixes maths in Loader::raster_bounds, so that it actually returns tight raster bounds.
Previously, it returned distored bounds due to usage of "RectF::set_origin_y"
which does not preserve Rect size.
@jrmuizel
Copy link
Collaborator

jrmuizel commented Apr 17, 2020

Can you add a test as well?

@ikatson
Copy link
Contributor Author

ikatson commented Apr 17, 2020

@jrmuizel I can surely try my best, can you please recommend what's an appropriate test for this use case?
For example, I could test that size of the rect with size 1.0 is comparable to the size of typographic_bounds(), but that would not test the actual origin.
Or I could just hard-code some expected raster_bounds for a sample glyph from a sample font - but then which font should I use?
Also this method is on the trait, but it's not used on Windows, so how should I go about testing on other platforms? (I only have a mac)
Thanks

@jrmuizel
Copy link
Collaborator

jrmuizel commented Apr 19, 2020

You can probably just hard code the expected bounds for a sample glyph of Inconsolata or EBGaramond12 which are already in the repository. The existing tests in src/test.rs make use of them and you should be able to just do something similar.

@ikatson ikatson force-pushed the ikatson:fix-raster-bounds-transform branch from b753684 to bb8b7f4 Apr 20, 2020
@ikatson ikatson force-pushed the ikatson:fix-raster-bounds-transform branch from bb8b7f4 to 27d77df Apr 20, 2020
@ikatson
Copy link
Contributor Author

ikatson commented Apr 20, 2020

@jrmuizel ready for review

@ikatson
Copy link
Contributor Author

ikatson commented Apr 23, 2020

@jrmuizel @pcwalton just checking if there's anything I need to update here for this to be good enough to merge

@pcwalton pcwalton merged commit 3f5c183 into servo:master Apr 23, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.