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

[3.x] Support multiline strings in buttons #41464

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

Waridley
Copy link
Contributor

@Waridley Waridley commented Aug 23, 2020

Added new String::split_lines and Font::get_multiline_string_size methods to make this and possible future features easier. Then I simply put the button text rendering logic in a for loop that loops over each line. Perhaps there's a more efficient method for rendering a block of text, but this changed as little code as possible, and I tried to make sure it was as efficient as I could.

Fixes #2967

@Waridley Waridley force-pushed the button_multiline_strings branch 2 times, most recently from 458fde4 to 8b5747b Compare August 23, 2020 03:21
@Chaosus Chaosus added this to the 4.0 milestone Aug 23, 2020
@akien-mga akien-mga requested a review from reduz September 3, 2020 08:37
@akien-mga
Copy link
Member

@bruvzg is working on a complex overhaul of text rendering for Godot 4.0 #41100 that will conflict and supersede with these changes, so they're unlikely to be merged in master.

There might be a case for adding it to 3.2 though, that can be discussed further.

@rmasp98
Copy link

rmasp98 commented Nov 28, 2020

@bruvzg is working on a complex overhaul of text rendering for Godot 4.0 #41100 that will conflict and supersede with these changes, so they're unlikely to be merged in master.

There might be a case for adding it to 3.2 though, that can be discussed further.

This information really should have been supplied in the original issue so that people don't waste there time trying to fix something that is never going to get merged

@Calinou
Copy link
Member

Calinou commented Nov 28, 2020

@rmasp98 This pull request could be redone from scratch for the 3.2 branch. In this case, it can probably be merged.

@Waridley If you have time, could you look into recreating this pull request based on the 3.2 branch? Thanks in advance 🙂

@Waridley Waridley force-pushed the button_multiline_strings branch from 8b5747b to 7642d2a Compare November 28, 2020 17:58
@Waridley Waridley changed the base branch from master to 3.2 November 28, 2020 18:00
@Waridley
Copy link
Contributor Author

Waridley commented Nov 28, 2020

Since I've been working with 3.2 anyway, I just switched this PR to the changes I've been working with.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 18, 2021

Actually I just noticed that this was for 3.x branch, sorry. The issue still exists in there.

@KoBeWi KoBeWi reopened this Nov 18, 2021
@KoBeWi KoBeWi removed the archived label Nov 18, 2021
@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@Waridley Waridley force-pushed the button_multiline_strings branch 2 times, most recently from fb73258 to d57495f Compare June 28, 2022 21:30
scene/resources/font.cpp Outdated Show resolved Hide resolved
core/ustring.cpp Outdated Show resolved Hide resolved
core/ustring.cpp Outdated Show resolved Hide resolved
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.

Tested and it works, code looks good overall (left some minor comments).

Needs a check from @bruvzg probably, for the String methods. I'm not sure about the \r thing.

@akien-mga akien-mga modified the milestones: 3.5, 3.x Jun 30, 2022
@akien-mga akien-mga requested a review from bruvzg August 25, 2022 19:47
@Waridley Waridley force-pushed the button_multiline_strings branch from d57495f to 8defa0f Compare September 21, 2022 03:18
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.

Not sure about adding String::split_lines and Font::get_multiline_string_size for a single use. Otherwise, code looks fine.

@Waridley
Copy link
Contributor Author

Not sure about adding String::split_lines and Font::get_multiline_string_size for a single use. Otherwise, code looks fine.

Would they potentially be worth exporting and documenting for users in GDScript? Or should I just inline them?

@akien-mga
Copy link
Member

Do we even need split_lines? I'm not sure \r is supposed for line break in Godot strings and so split("\n") should be sufficient.

@Waridley Waridley force-pushed the button_multiline_strings branch from 8defa0f to 199f3d9 Compare October 2, 2022 02:51
@Waridley
Copy link
Contributor Author

Waridley commented Oct 2, 2022

Do we even need split_lines? I'm not sure \r is supposed for line break in Godot strings and so split("\n") should be sufficient.

You're right, looks like all the places that deal with '\r' are parsing external files, not Object properties.

@akien-mga akien-mga merged commit f793f20 into godotengine:3.x Oct 3, 2022
@akien-mga akien-mga modified the milestones: 3.x, 3.6 Oct 3, 2022
@akien-mga
Copy link
Member

Thanks!

@Waridley Waridley deleted the button_multiline_strings branch October 3, 2022 19:21
@akien-mga akien-mga changed the title Support multiline strings in buttons [3.x] Support multiline strings in buttons Sep 9, 2024
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.

Button text ignores breakline (\n) (fixed in master)
9 participants