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

VTab and underlined bug in Text class #442

Closed
quinor opened this Issue Jul 31, 2013 · 13 comments

Comments

Projects
None yet
5 participants
@quinor

quinor commented Jul 31, 2013

Text text("It is an example\vusing vtab");
text.style |= Text.Underlined;
window.draw(text);
window.display();

If you use vtab with Text.Underlined, the line under a text isn't broken at "\v" character but is rather on the level of the second part of the line.

@ghost ghost assigned LaurentGomila Jul 31, 2013

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 2, 2013

Contributor

I can confirm this bug. I attached a screenshot.
If nobody minds I would like to work on this bug and see if I can find a solution.

bug

Contributor

Foaly commented Aug 2, 2013

I can confirm this bug. I attached a screenshot.
If nobody minds I would like to work on this bug and see if I can find a solution.

bug

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 2, 2013

Member

The fix should be straight-forward. Currently, underlines are added at each \n and at the end of the string; added them also on \v should solve this problem.

Member

LaurentGomila commented Aug 2, 2013

The fix should be straight-forward. Currently, underlines are added at each \n and at the end of the string; added them also on \v should solve this problem.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 3, 2013

Contributor

Alright I wrote a fix for this issue. It's in the pull request for the strikethough text style. It's tested and works. Here is a screen shot:
fixed

Contributor

Foaly commented Aug 3, 2013

Alright I wrote a fix for this issue. It's in the pull request for the strikethough text style. It's tested and works. Here is a screen shot:
fixed

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 3, 2013

Member

It's in the pull request for the strikethough text style

If you want me to apply the fix quickly, you'd rather put it in a separate pull request.

Member

LaurentGomila commented Aug 3, 2013

It's in the pull request for the strikethough text style

If you want me to apply the fix quickly, you'd rather put it in a separate pull request.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 3, 2013

Contributor

Sorry I don't have time right now. I'll try to make a seperate pull request next week. But what speaks against merging the other pull request?

Contributor

Foaly commented Aug 3, 2013

Sorry I don't have time right now. I'll try to make a seperate pull request next week. But what speaks against merging the other pull request?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 3, 2013

Member

Because I'm only 99% sure that I want to add this feature. I need 100% ;)

Generally speaking, it's not a good idea to mix unrelated things in a single pull request. It just makes things worse for me.

Member

LaurentGomila commented Aug 3, 2013

Because I'm only 99% sure that I want to add this feature. I need 100% ;)

Generally speaking, it's not a good idea to mix unrelated things in a single pull request. It just makes things worse for me.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 3, 2013

Contributor

Ok i understand that. It makes sense. But thats why i asked beforehand if its ok if i put them in the same pull request...
Anyway I'll make a seperate one this week.
May I ask what's the last percent missing? The feature has been asked for a couple times and it would be a nice addition to the SFML text rendering. Other libraries implement it too.

Contributor

Foaly commented Aug 3, 2013

Ok i understand that. It makes sense. But thats why i asked beforehand if its ok if i put them in the same pull request...
Anyway I'll make a seperate one this week.
May I ask what's the last percent missing? The feature has been asked for a couple times and it would be a nice addition to the SFML text rendering. Other libraries implement it too.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 3, 2013

Member

But thats why i asked beforehand if its ok if i put them in the same pull request...

Ah, sorry, I misundertood. I thought you were just asking if you could write the fix yourself, not if you could do it in the existing pull request.

May I ask what's the last percent missing?

I don't know. Just taking 5 minutes to think about it ;)
There are so many tasks pending for SFML, sometimes I leave them in the list and keep the final decision for later.

Member

LaurentGomila commented Aug 3, 2013

But thats why i asked beforehand if its ok if i put them in the same pull request...

Ah, sorry, I misundertood. I thought you were just asking if you could write the fix yourself, not if you could do it in the existing pull request.

May I ask what's the last percent missing?

I don't know. Just taking 5 minutes to think about it ;)
There are so many tasks pending for SFML, sometimes I leave them in the list and keep the final decision for later.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 9, 2013

Contributor

Ah, sorry I misunderstood.

No problem. That happens...

So did you find the 5 minutes in the last couple days? :)
If not, I'll make a new seperate commit.

Contributor

Foaly commented Aug 9, 2013

Ah, sorry I misunderstood.

No problem. That happens...

So did you find the 5 minutes in the last couple days? :)
If not, I'll make a new seperate commit.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 9, 2013

Member

Please do it, even if I accept both. Don't wait for me ;)

Member

LaurentGomila commented Aug 9, 2013

Please do it, even if I accept both. Don't wait for me ;)

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 9, 2013

Contributor

Alright!

Contributor

Foaly commented Aug 9, 2013

Alright!

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 9, 2013

Contributor

There is the seperate one.

Contributor

Foaly commented Aug 9, 2013

There is the seperate one.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r
Member

eXpl0it3r commented May 13, 2014

Merged c45039e

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