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

In nodes RichTextLabel and Label, get_total_character_count() now has a parameter to include or not spaces #52712

Closed
wants to merge 1 commit into from

Conversation

Lucas3H
Copy link

@Lucas3H Lucas3H commented Sep 15, 2021

The default value of this new parameter is true.

This was made mainly because the behavior of the function was inconsistent with the description in the documentation.

Fixes #27896.

@Lucas3H Lucas3H requested review from a team as code owners September 15, 2021 16:22
@YeldhamDev YeldhamDev added this to the 4.0 milestone Sep 15, 2021
@YeldhamDev
Copy link
Member

@Lucas3H Lucas3H force-pushed the character-count branch 3 times, most recently from ec3338d to d20a65f Compare September 15, 2021 18:20
doc/classes/Label.xml Outdated Show resolved Hide resolved
<description>
Returns the total number of characters from text tags. Does not include BBCodes.
If [code]include_spaces[/code] is set to [code]false[/code], it doesn't count spaces.
It's value is true by default.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

scene/gui/label.cpp Outdated Show resolved Hide resolved
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

The code is fine, but I do not see why this is useful to have.
Not counting spaces in the 3.x Label is probably a bug.

@Lucas3H
Copy link
Author

Lucas3H commented Sep 22, 2021

The code is fine, but I do not see why this is useful to have.
Not counting spaces in the 3.x Label is probably a bug.

In fact, it doesn't look useful to add this new parameter. The reason I did it was because it shouldn't count spaces, accordingly to the previous documentation, but it was counting. It would be simpler to just fix the documentation, but maybe there are some places where this function was used supposing it is counting spaces and some places where it isn't.

Is it better to just fix the documentation?

But yeah, definitely a PR in the 3.x would be way more clear that it is fixing a bug. Can I do this already? I read that all fixes should be done first in master branch.

@Lucas3H Lucas3H requested a review from bruvzg September 23, 2021 17:31
@YuriSizov
Copy link
Contributor

So it seems that there is no need to change anything in the master branch. A fix for a bug in 3.x doesn't need to have a master counterpart, so a direct PR can be opened. For either branch if we can improve the docs, a new PR doing that is welcome 🙂

@YuriSizov YuriSizov closed this Jan 19, 2023
@YuriSizov YuriSizov removed this from the 4.0 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spaces are counted in RichTextLabel but not Label when using get_total_character_count()
5 participants