-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement SvgTextSizeCalculator #97
Implement SvgTextSizeCalculator #97
Conversation
ab0a3bf
to
a541299
Compare
I'm going to review this code in the coming days, maybe it will be slightly modified. I really appreciate your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is OK for me, but it needs to be tested
I'm working on a new more cleaner implementation. Will return when it will be done. |
7c44cae
to
49bccd4
Compare
@garak I've reworked this PR. All the logic of parsing SVG fonts and unicode characters (code points) extracted to separated unit tested packages:
If you are OK with such solution I will continue to work towards this implementation. Motivation behind this change: reading and parsing SVG fonts and unicode strings not that easy as it might look like. For example current solution does not allow us to use composition characters like composable emojis. We may add support later in separate package and do not change this package code, because solution may be very complicated. Things to do before merge:
|
I have one question about GD calculator. There is a code: public function calculateWidth(string $text, int $size = self::TEXT_SIZE): float
{
$size = $this->convertToPt($size);
$box = \imagettfbbox($size, 0, $this->fontPath, $text);
return \round(\abs($box[2] - $box[0]) + self::SHIELD_PADDING_EXTERNAL + self::SHIELD_PADDING_INTERNAL, 1);
}
private function convertToPt(int $pixels): float
{
return \round($pixels * 0.75, 1);
} Why do we decreasing font size using Update: I suppose I found an answer. GD1 used pixels as first argument, GD2 uses points. |
f7a3e6d
to
bbadaa4
Compare
Tests are failing on code style. Same as in #101. It looks like CS Fixer issue, need fix it and re-run here. |
@garak I wrote tests for the new SvgTextCalculator and for GdTextCalculator (they were missing). |
On a second thought, probably it's useless to have the CS checked inside versions. Please remove that part, let's keep only the initial global CS check |
@garak what part are you talking about? CS fails on infrastructure code, not committed one. |
I'm fixing it here: #102 |
Please rebase! ;) |
but you fixed the symptom, not the problem itself :-( |
bbadaa4
to
8ee8901
Compare
@garak @JellyBellyDev rebased, ready for review |
it is good as temporary solution, but later could be changed as well |
@garak @JellyBellyDev who can make minor release? |
Done! 😉 |
@JellyBellyDev release does not appeared in Packagist: https://packagist.org/packages/badges/poser |
Now yes! 👍🏻 |
I always forget that I don't have the privileges to set the package self-update hook on packagist and with each release I have to update it manually |
This is only the first step towards stopping using GD for icon rendering. To completely remove the GD dependency, we need to make backward incompatible changes.
This PR introduces new SvgTextSizeCalculator class which will allow us to make proof of concept in a safe manner. In a future major versions we could define it as default fallback calculator.
Calculate badge width using GD (default):
Calculate badge width using SVG (new):
Note:
SvgForTheBadgeRenderer
using its own class to compute text width using EasySVG (where I got an inspiration), but it using GD too because of the parent method call. So to avoid using GD you should provide new calculator implementation as second argument.