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

Strikethrough text style #362

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Foaly
Contributor

Foaly commented Mar 6, 2013

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

I had to resend this pull request, for reasons I explained in the old pull request (#352). Sorry again. This one is tested and works. Promise :)

@germinolegrand

View changes

Show outdated Hide outdated include/SFML/Graphics/Text.hpp
@germinolegrand

View changes

Show outdated Hide outdated src/SFML/Graphics/Text.cpp
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 6, 2013

Contributor

Damn you typos! :D
I don't know how I should fix this. Should I delete this commit (using commit rebase) and commit a fixed version. That's how i did it before, it leaves a clean commit that Laurent can merge with on click, but we would probably loose part of the discussion. Or should I push a new commit that changes the two typos?

Contributor

Foaly commented Mar 6, 2013

Damn you typos! :D
I don't know how I should fix this. Should I delete this commit (using commit rebase) and commit a fixed version. That's how i did it before, it leaves a clean commit that Laurent can merge with on click, but we would probably loose part of the discussion. Or should I push a new commit that changes the two typos?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 6, 2013

Member

I'd say make another commit.
Some people hate it, but you could also do a forced push. It's usually a bad practice if you were to push into the master, but might be more or less acceptable for a pull request...

git add fileXYZ
git commit --amend
git push -f

Member

eXpl0it3r commented Mar 6, 2013

I'd say make another commit.
Some people hate it, but you could also do a forced push. It's usually a bad practice if you were to push into the master, but might be more or less acceptable for a pull request...

git add fileXYZ
git commit --amend
git push -f

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 6, 2013

Contributor

Ok perfect! That went very smooth. Thanks for the help!
Now we'll see what Laurent says :)

Contributor

Foaly commented Mar 6, 2013

Ok perfect! That went very smooth. Thanks for the help!
Now we'll see what Laurent says :)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 7, 2013

Member

If strikethrough is a made of two words (I think it is), then it should be an upper case T (StrikeThrough).

And what's the reason for putting the line at 40% of the character height? Is it just something that looks good, or is there some kind of standard value?

Member

LaurentGomila commented Mar 7, 2013

If strikethrough is a made of two words (I think it is), then it should be an upper case T (StrikeThrough).

And what's the reason for putting the line at 40% of the character height? Is it just something that looks good, or is there some kind of standard value?

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 7, 2013

Contributor

According to my dictionary it is two words. So I should change it to sf::Text::StrikeThrough, right?

Back when I first made this I did some research, but I couldn't find a "standart height" for putting the line. I first assumend the line should be at 50% of the characters height, but that didn't look good. I tested with 10+ standart and non-standart fonts and 40% looked the best.

edit: I just found this article that says the traditional height is 70-90% of the x characters height. My code is within that range.

Contributor

Foaly commented Mar 7, 2013

According to my dictionary it is two words. So I should change it to sf::Text::StrikeThrough, right?

Back when I first made this I did some research, but I couldn't find a "standart height" for putting the line. I first assumend the line should be at 50% of the characters height, but that didn't look good. I tested with 10+ standart and non-standart fonts and 40% looked the best.

edit: I just found this article that says the traditional height is 70-90% of the x characters height. My code is within that range.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 7, 2013

Member

According to my dictionary it is two words. So I should change it to sf::Text::StrikeThrough, right?

Yes.

I just found this article that says the traditional height is 70-90% of the x characters height. My code is within that range.

Great :)

Member

LaurentGomila commented Mar 7, 2013

According to my dictionary it is two words. So I should change it to sf::Text::StrikeThrough, right?

Yes.

I just found this article that says the traditional height is 70-90% of the x characters height. My code is within that range.

Great :)

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

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 7, 2013

Contributor

Alright I have fixed the typos and changed the variable name. So we have one clean commit now!

Contributor

Foaly commented Mar 7, 2013

Alright I have fixed the typos and changed the variable name. So we have one clean commit now!

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 7, 2013

Member

Thanks, it will probably be integrated to SFML 2.1.

Member

LaurentGomila commented Mar 7, 2013

Thanks, it will probably be integrated to SFML 2.1.

@ghost ghost assigned LaurentGomila Mar 7, 2013

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 7, 2013

Contributor

Awesome! Looking forward to it.
Just out of curiosity may I ask why 2.1? It doesn't break the current API or anything.

Contributor

Foaly commented Mar 7, 2013

Awesome! Looking forward to it.
Just out of curiosity may I ask why 2.1? It doesn't break the current API or anything.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 7, 2013

Member

I know, but SFML 2.0 is so close now that I prefer to keep that for the next minor release. Everything is ready for 2.0, only straight-forward stuff can be added to it.

Member

LaurentGomila commented Mar 7, 2013

I know, but SFML 2.0 is so close now that I prefer to keep that for the next minor release. Everything is ready for 2.0, only straight-forward stuff can be added to it.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 7, 2013

Contributor

Even better, if that means that 2.0 is about to be released! :)

Contributor

Foaly commented Mar 7, 2013

Even better, if that means that 2.0 is about to be released! :)

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly May 10, 2013

Contributor

Since SFML 2.0 is released, I think you can now merge this pull request. What do you think?

Contributor

Foaly commented May 10, 2013

Since SFML 2.0 is released, I think you can now merge this pull request. What do you think?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 11, 2013

Member

There's no need to add a comment to all the pull requests now that SFML 2.0 has been released.

I know about these requests, I have reviewed them all after the release, and I have already chosen when they will be merged. As you can see, this one will not be part of SFML 2.1, because SFML 2.1 will be a bug fix release. No new feature.

Member

LaurentGomila commented May 11, 2013

There's no need to add a comment to all the pull requests now that SFML 2.0 has been released.

I know about these requests, I have reviewed them all after the release, and I have already chosen when they will be merged. As you can see, this one will not be part of SFML 2.1, because SFML 2.1 will be a bug fix release. No new feature.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly May 11, 2013

Contributor

Sorry I didn't mean to offend anybody. I was just going through some of the old stuff and wondered what happened to it and if it got forgotten. I didn't know you already reviewed all of them. If you say 2.1 is just bugfixes, that's cool with me.

Contributor

Foaly commented May 11, 2013

Sorry I didn't mean to offend anybody. I was just going through some of the old stuff and wondered what happened to it and if it got forgotten. I didn't know you already reviewed all of them. If you say 2.1 is just bugfixes, that's cool with me.

Foaly added some commits Aug 3, 2013

Fix for VTab and underline bug
Fixes issue #442 and moves code for line creation in a seperate method
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 3, 2013

Contributor

OK. I the first commit I wrote fixes #442. I tested it and it works as expected (for a screenshot take a look at the original issue). The second commit updates the branch to the current state of the official repository (because it was already behind quiet a few commits). I also resolved a merge conflict in it, caused by the recent change through commit 5c431b4. I recompiled it afterwards and it works fine.

Contributor

Foaly commented Aug 3, 2013

OK. I the first commit I wrote fixes #442. I tested it and it works as expected (for a screenshot take a look at the original issue). The second commit updates the branch to the current state of the official repository (because it was already behind quiet a few commits). I also resolved a merge conflict in it, caused by the recent change through commit 5c431b4. I recompiled it afterwards and it works fine.

@LaurentGomila LaurentGomila removed their assignment May 19, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 22, 2014

Member

This PR needs updating, due to the fact that VTab (c45039e) has been removed.

Member

eXpl0it3r commented May 22, 2014

This PR needs updating, due to the fact that VTab (c45039e) has been removed.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 18, 2014

Member

Updated and squashed commit at 5f3b6cb.

Member

binary1248 commented Jul 18, 2014

Updated and squashed commit at 5f3b6cb.

@binary1248 binary1248 self-assigned this Aug 15, 2014

@binary1248 binary1248 added s:accepted and removed s:unassigned labels Aug 15, 2014

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 18, 2014

Member

Superseded by #682.

Member

binary1248 commented Aug 18, 2014

Superseded by #682.

@binary1248 binary1248 closed this Aug 18, 2014

@binary1248 binary1248 added s:superseded and removed s:accepted labels Aug 18, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.x, 2.2 Aug 19, 2014

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