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

Added a strikethrough text style to sf::Text. Fixes issue #243. #682

Merged
merged 1 commit into from Aug 19, 2014

Conversation

Projects
None yet
3 participants
@binary1248
Member

binary1248 commented Aug 18, 2014

Implements a StrikeThrough style for sf::Text (#243). This is basically a patched version of #362.

@binary1248 binary1248 self-assigned this Aug 18, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 18, 2014

Member

This PR has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

Edit: Here's a test example, which works fine.

Member

eXpl0it3r commented Aug 18, 2014

This PR has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

Edit: Here's a test example, which works fine.

@eXpl0it3r eXpl0it3r merged commit 5f3b6cb into master Aug 19, 2014

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone Aug 19, 2014

@eXpl0it3r eXpl0it3r deleted the feature/strikethrough branch Aug 19, 2014

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 20, 2014

Contributor

Well I have two things, but I guess I wasn't quick enough... :D
First thing is minor. I think underlineThickness should be renamed to something like lineThickness, because it is used for both the underline and the strike through.
Second thing is I think the code that appends the vertices for the line (for example lines 315-320 in Text.cpp) should be moved into a separate function, because the exact same code is repeated four times in that method.

Contributor

Foaly commented Aug 20, 2014

Well I have two things, but I guess I wasn't quick enough... :D
First thing is minor. I think underlineThickness should be renamed to something like lineThickness, because it is used for both the underline and the strike through.
Second thing is I think the code that appends the vertices for the line (for example lines 315-320 in Text.cpp) should be moved into a separate function, because the exact same code is repeated four times in that method.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 20, 2014

Member

underlineThickness should be renamed to something like lineThickness

underlineThickness was not named lineThickness because it is taken directly from the font metrics. It is the correct thickness... for the underline, whereas the font does not tell us anything about the thickness that a strikethrough should have. So in this case we are just re-using the underline thickness for the strikethrough as well, which is reflected in the variable name.

Second thing is I think the code that appends the vertices for the line (for example lines 315-320 in Text.cpp) should be moved into a separate function, because the exact same code is repeated four times in that method.

Nope, if you look carefully it isn't the exact same code. Even if just the initial variable assignments change, such a function would require more parameters than necessary and make it less sensible to pull it out into a separate function. It is easier to read and understand like that, and the compiler optimizes where it thinks it is necessary anyway.

Member

binary1248 commented Aug 20, 2014

underlineThickness should be renamed to something like lineThickness

underlineThickness was not named lineThickness because it is taken directly from the font metrics. It is the correct thickness... for the underline, whereas the font does not tell us anything about the thickness that a strikethrough should have. So in this case we are just re-using the underline thickness for the strikethrough as well, which is reflected in the variable name.

Second thing is I think the code that appends the vertices for the line (for example lines 315-320 in Text.cpp) should be moved into a separate function, because the exact same code is repeated four times in that method.

Nope, if you look carefully it isn't the exact same code. Even if just the initial variable assignments change, such a function would require more parameters than necessary and make it less sensible to pull it out into a separate function. It is easier to read and understand like that, and the compiler optimizes where it thinks it is necessary anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment