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

Remove vertical scrollbar padding from line width calc #83286

Merged

Conversation

bronsonholden
Copy link
Contributor

@bronsonholden bronsonholden commented Oct 13, 2023

Hello 👋

Fix for text edits occasionally rendering a horizontal scrollbar when not necessary, causing what I can only describe as a jitter: sometimes it appears, sometimes not—depending on the text edit's width.

This appears to be caused by the calculation to determine if the max line width exceeds the visible area also accounting for the presence of the vertical scrollbar. As I understand it, this isn't necessary since if the text area wraps, the _update_wrap_at_column function will ensure the line stays within the visible area (which includes the vertical scrollbar, if visible).

And for text fields with no wrapping (specifically I'm thinking of the script editor), ignoring the vertical scrollbar width in this calculation doesn't cause clipped text because lines already are given some extra space on L7283

Before

hscroll-before.mov

After

hscroll-after.mov

Closes #83186

Visibility of the vertical scrollbar is already accounted for in
`_update_wrap_at_column` which in turn affects max line width of
the text area.
@bronsonholden bronsonholden requested a review from a team as a code owner October 13, 2023 18:11
@KoBeWi KoBeWi added this to the 4.2 milestone Oct 13, 2023
@bronsonholden
Copy link
Contributor Author

@KoBeWi a test for expected horizontal scroll position is failing, likely since the width of the text is reduced with this change. Is there a way to run the tests visually to verify changing the test expectation to what it now receives (306) doesn't gloss over visual or functional issues with the text edit field itself?

@bronsonholden bronsonholden requested a review from a team as a code owner October 13, 2023 19:23
@bronsonholden
Copy link
Contributor Author

@KoBeWi I added a check that "justifies" the update to 306 as the expected horizontal scroll position since the change just removes the v-scroll bar's width from the total width of rendered text. Let me know if you think there's a better approach. We can remove the newly added test if approved, though. It's really just a sanity check for changing 314 → 306

@KoBeWi
Copy link
Member

KoBeWi commented Oct 16, 2023

Is there a way to run the tests visually to verify changing the test expectation to what it now receives (306) doesn't gloss over visual or functional issues with the text edit field itself?

No, you'd have to copy the code e.g. into some part of the editor, or translate it to GDScript. In this case it's easier to use print_line() to see the expected value.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The vmin.x was added in 26a7152, but removing it does not seem to reintroduce the original issue nor cause other adverse effects.

@akien-mga akien-mga merged commit 3e8dc9d into godotengine:master Oct 17, 2023
15 checks passed
@akien-mga
Copy link
Member

akien-mga commented Oct 17, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

For future contributions, please keep them as a single commit, especially when the follow-up commit is a fixup to a problem in the first one.

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.

Horizontal scrollbar in multiline text editing
3 participants