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

RichTextLabel: Add count and scroll functions for wrapped lines and paragraphs. #45128

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 12, 2021

Adds separate get_total_x_count, get_visible_x_count and scroll_to_x functions for wrapped lines and paragraphs (newlines).

Old behavior: get_line_count and scroll_to_line functions were counting newlines/paragraphs, get_visible_line_count was counting lines before #40217, and paragraphs after.

New behavior: x_paragraph_x functions count paragraphs (newlines or [p] tags), and x_line_x functions count wrapped lines.

Fixes: #40982

@akien-mga
Copy link
Member

CC @KoBeWi @theoway

@KoBeWi
Copy link
Member

KoBeWi commented Jan 13, 2021

I can't check if it fixes my issue, because percent_visible doesn't seem to work.

@bruvzg bruvzg marked this pull request as ready for review January 18, 2021 06:51
@bruvzg bruvzg requested a review from a team as a code owner January 18, 2021 06:51
…t` and `scroll_to_x` functions for wrapped lines and paragraphs (newlines).
@akien-mga akien-mga merged commit 16fa420 into godotengine:master Jan 18, 2021
@akien-mga
Copy link
Member

Thanks!

Comment on lines 3417 to 3423
int RichTextLabel::get_line_count() const {
return current_frame->lines.size();
int line_count = 0;
for (int i = 0; i < main->lines.size(); i++) {
line_count += main->lines[i].text_buf->get_line_count();
}
return line_count;
}
Copy link
Contributor

@EricEzaM EricEzaM May 25, 2021

Choose a reason for hiding this comment

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

I think this is wrong, or should at least be made optional.

The actual amount of lines in a RTL and the amount of lines rendered is two different things.

When you want to get the line count of the number of lines the actual RLT contains, for example to remove a line in remove_line(), you want the value of current_frame->lines.size(), not including wrapped lines. If you include wrapped lines, you may be reporting a larger number of lines than actually exists. In which case you will exit out of remove_line() early, since p_line > current_frame->lines.size()

I dont' really know the usecase for the number of lines rendered, which is what this PR does. But I think get_line_count() should at least have a parameter to determine which value to obtain.

Copy link
Member Author

Choose a reason for hiding this comment

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

get_paragraph_count should return number of lines without counting wrapped lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see... sorry, didn't catch that.

Were uses of the "legacy" get_line_count() changed to get_paragraph_count() within the rest of the codebase?

I'm also not sure about the naming... but I guess it works and I don't have better suggestions at this stage.

@harrisyu
Copy link
Contributor

Can this port to 3.x ?

@Valla-Chan
Copy link

seconding this, this is still a huge issue in 3.x

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.

RichTextLabel doesn't count wrapped lines properly
6 participants