Fixing ReferenceError: totalHeight is not defined throw by textAlign(…

[malviys]

Fixing ReferenceError: totalHeight is not defined throw by textAlign(, CENTER) and textAlign(, BOTTOM)
Resolves #5360

Changes:
Removed undefine variable totalHeight from src/core/p5.Renderer.js causing ReferenceError while calling
textAlign_(, CENTER) and textAlign(__, BOTTOM)

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[welcome]

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

[outofambit]

@malviys thank you for working on this! could you please include screenshots of using textAlign to help confirm that we now have the correct behavior (besides not erroring)? thank you!

(also ccing @lawreka in case you have thoughts on the approach here!)

[lawreka]

Thanks for tagging me here, @outofambit ! It looks like I left these uninitialized variables in when refactoring the text wrapping :(

This fix looks like it does eliminate the ReferenceError, but I'm digging in a little bit into #5146 to see if I missed something important about how totalHeight was used. I would expect in this example that if the text is vertically aligned to the center of the box (where the line is drawn through), the whole multi-line block of text would shift up a little bit.

As it is, only the first line is conforming to the vertical-alignment rule. I would assume also with text-align set to BOTTOM that we don't see the last line aligned with the bottom, but the first line.

If you want to get rid of the error quickly and plan to release soon, this is a good fix, but I'm also happy to revisit the text wrapping function and see what got lost in re-implementation.

[malviys]

Here is code screenshot of textAlign. Putting CENTER or BOTTOM as any parameter throws ReferenceError, now it is working fine. @lawreka I checked in source code that variable totalHeight is not declared and used anywhere.

[Snowman-s]

I had been struggling with this problem for a while. Thanks for coming up with a solution!

Here are my thoughts:

Presumably, the totalHeight was the actual height of the text as it was drawn.

This value would have been used to draw the entire text across multiple lines at the specified position, not just a single line.
(In fact, before #5146, totalHeight was declared and used.)

I noticed that it is not easy to calculate the totalHeight, because the height of the text changes for each textWrapStyle.
You probably need to split the text and calculate what the height will be beforehand (if you use totalHeight).

[Snowman-s]

@malviys @lawreka,

It would be great if you could tell me what the current status of this Pull Request is. Thanks!

[lawreka]

You probably need to split the text and calculate what the height will be beforehand

Yes, this would require a more in-depth solution, which would probably necessitate its own PR separate from this one. I thought I would have time to work on that when I commented 17 days ago but unfortunately this was not the case :(

what the current status of this Pull Request is

I think that will be up to @outofambit to approve, I'm happy with @malviys solving the error in this PR and to open a new issue/follow-up PR to actually implement the correct vertical alignment.

[outofambit]

hey @lawreka @Snowman-s @malviys! thanks for the great conversation here. it seems like we should merge this PR to fix the immediate error and then follow up with more work to re-implement totalHeight for multiline bottom aligned text areas. i'll merge this now and file an issue to followup on the second part. thanks again!