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: TextBoxComponent rendering for new line #2413

Merged
merged 3 commits into from Mar 17, 2023

Conversation

immadisairaj
Copy link
Contributor

@immadisairaj immadisairaj commented Mar 16, 2023

Initially the calculation of lines and updating accordingly ignores the case for a new line escape characters.
This PR changes how we calculate and update lines when checking for bounds.

Description

Before:
When a text that contains \n character is given to the TextBox for example Hello \n World. It doesn't consider the new line and just appends the \n to the same line. This increases the lineHeight for that particular word (and won't be updated in this case). But, instead when there is a text \nHello World, then the initial lineHeight itself is twice than what it actually is.

So, the main problem here is that we are not considering (ignoring) the \n characters.

This can be solved by checking for any new lines in the split word (From split(' ')) by using split('\n') and adding the lines appropriately into the _lines.

After:
Now, whenever a \n is encountered, we respect both the new line and the spacing right after the \n character. This eliminates the improper rendering of the text. Also, lineHeight will always be same as expected and not more.

Screenshots

Input1: Hello \n something great World
Output1:
Screenshot 2023-03-17 at 15 18 54

Input2: \nHello\nsomething great World'
Output2:
Screenshot 2023-03-17 at 15 21 07

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Migration instructions

  • [-] Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Closes #1622

This commit changes how we calculate and fill lines when checking for
bounds
@immadisairaj immadisairaj changed the title fix: text box component rendering for new line fix: TextBoxComponent rendering for new line Mar 16, 2023
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, could you add a test?

@immadisairaj
Copy link
Contributor Author

Great. I will add. So, the reason I put it as draft.

@immadisairaj
Copy link
Contributor Author

Should I be using the golden tests? if so, what should be my approach in generating images for it?

@spydon
Copy link
Member

spydon commented Mar 17, 2023

Should I be using the golden tests? if so, what should be my approach in generating images for it?

If you don't really want to, I think you can skip using goldens for this change. I think it's enough to just verify that the height is correct, and possibly verify that the list has the correct words on each line. To do the last part you'd have to move out the list from the method and expose it with @visibleForTesting.

@immadisairaj
Copy link
Contributor Author

If you don't really want to, I think you can skip using goldens for this change.

I can use the goldens, that's not a problem just that (I can refer the already present test), is it just writing the app and screenshotting it or is there any other way for it?

I think it's enough to just verify that the height is correct,

This I can do.

and possibly verify that the list has the correct words on each line. To do the last part you'd have to move out the list from the method and expose it with @visibleForTesting.

The _lines is already outside the method. But, it's a private field, I don't think it is good to make it public (to add @visibleForTesting)?

I am ready with anything what you suggest.

@spydon
Copy link
Member

spydon commented Mar 17, 2023

The _lines is already outside the method. But, it's a private field, I don't think it is good to make it public (to add @visibleForTesting)?

Setting it as public but with the visibleForTesting tag is virtually the same as having it as private, so it should be fine. :)

@immadisairaj immadisairaj marked this pull request as ready for review March 17, 2023 10:28
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@spydon spydon merged commit 9008998 into flame-engine:main Mar 17, 2023
6 checks passed
@immadisairaj immadisairaj deleted the text-box branch March 17, 2023 13:39
@st-pasha
Copy link
Contributor

I can use the goldens, that's not a problem just that (I can refer the already present test), is it just writing the app and screenshotting it or is there any other way for it?

For future: the testGolden method contains an explanation for how to set up and use golden tests:
https://github.com/flame-engine/flame/blob/main/packages/flame_test/lib/src/test_golden.dart

@immadisairaj
Copy link
Contributor Author

For future: the testGolden method contains an explanation for how to set up and use golden tests: https://github.com/flame-engine/flame/blob/main/packages/flame_test/lib/src/test_golden.dart

Wow. I literally missed it. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextBoxComponent overwrites lines if the text contains newlines
4 participants