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 get_visible_line_count in RichTextLabel #101205

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

voylin
Copy link
Contributor

@voylin voylin commented Jan 7, 2025

Fixes #75847

The _draw_line() function in RichTextLabel was adding to line_count, no matter how many lines were actually visible. This made the function get_visible_line_count() to return an incorrect amount. For visible_characters = 0 it would return 0, but setting it any higher would return a line count for all lines, even ones which weren't visible.

I hope this is enough to fix and close #75847

@voylin voylin requested a review from a team as a code owner January 7, 2025 02:40
@AThousandShips AThousandShips added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jan 7, 2025
@AThousandShips AThousandShips added this to the 4.4 milestone Jan 7, 2025
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.

This looks OK, but will also change behavior on completely empty lines (which were counted before), and I'm not sure if it's desired.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 7, 2025

This looks OK, but will also change behavior on completely empty lines (which were counted before), and I'm not sure if it's desired.

You mean they are no longer counted?
I tested and it seems to work, at least partially

test

test

correctly returns 3 lines.

test


incorrectly (?) returns 3 lines. Looks like if the text ends with empty line, it will be ignored. All other empty lines are properly counted.

@voylin voylin changed the title Fix get_visible_lint_count in RichTextLabel Fix get_visible_line_count in RichTextLabel Jan 7, 2025
@bruvzg
Copy link
Member

bruvzg commented Jan 8, 2025

Looks like if the text ends with empty line, it will be ignored. All other empty lines are properly counted.

All other lines except last probably aren't fully empty (contain line break character).

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.

Old code was not counting the last empty line either, so this should not be a concern (at least not in this PR context).

@voylin voylin force-pushed the get_visible_line_count_fix branch from 3708870 to 3e333df Compare January 9, 2025 01:25
@voylin
Copy link
Contributor Author

voylin commented Jan 9, 2025

This looks OK, but will also change behavior on completely empty lines (which were counted before), and I'm not sure if it's desired.

I just tested this but empty lines are still being counted, so not really an issue.
image
I'm just printing the visible lines in _process for this quick test.

@akien-mga akien-mga changed the title Fix get_visible_line_count in RichTextLabel Fix get_visible_line_count in RichTextLabel Jan 9, 2025
@akien-mga akien-mga merged commit 2e657bf into godotengine:master Jan 9, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichTextLabel get_visible_line_count() does not work with visible_characters anymore
5 participants