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 clear() fails #54951

Open
Koyper opened this issue Nov 13, 2021 · 11 comments
Open

RichTextLabel clear() fails #54951

Koyper opened this issue Nov 13, 2021 · 11 comments

Comments

@Koyper
Copy link
Contributor

Koyper commented Nov 13, 2021

Godot version

3.4 stable

System information

OSX

Issue description

When creating multiple instances containing a RichTextLabel in a for statement, clear() at _ready() and _enter_tree() fails to clear the placeholder text in bbcode_text

This worked in 3.3 stable.

Steps to reproduce

Create a scene with a RTL and placeholder bb code in bbcode_text that calls clear() at _ready();

In a parent scene, add multiple instances of the above scene to a grid container;

You should see the placeholder bbcode_text instead of empty bbcode_text.

Minimal reproduction project

No response

@Calinou
Copy link
Member

Calinou commented Nov 13, 2021

@Koyper Please upload a minimal reproduction project to make this easier to troubleshoot.

@Koyper
Copy link
Contributor Author

Koyper commented Nov 13, 2021

RichTextLabel_clear_bug2.zip

Running in 3.3 stable returns a cleared panel - running in 3.4 stable returns tiles of placeholder text.

@Koyper
Copy link
Contributor Author

Koyper commented Nov 13, 2021

Simply removing the theme resource setting in the parent scene causes it to work correctly.

@Koyper
Copy link
Contributor Author

Koyper commented Nov 13, 2021

Additionally, by initial steps will not reproduce the bug - I had to work to strip it down to the minimal to find out what would reproduce it...

@Koyper
Copy link
Contributor Author

Koyper commented Nov 13, 2021

...my initial steps...

@timothyqiu
Copy link
Member

timothyqiu commented Nov 14, 2021

This is an regression since 3.3.3. clear() here succeeded, but the tag stack is built again during the THEME_CHANGED notification.

clear() never set bbcode_text to an empty string (but stated to in the doc), and #49174 introduces the behavior of rebuilding the tag stack with bbcode_text when theme changed.

So, should clear() set bbcode_text to empty?

@akien-mga
Copy link
Member

So, should clear() set bbcode_text to empty?

I guess yeah.

@timothyqiu
Copy link
Member

After a bit more inspection, it seems RichTextLabel don't expect clear() to touch bbcode_text. e.g.

void RichTextLabel::set_bbcode(const String &p_bbcode) {
bbcode = p_bbcode;
if (is_inside_tree() && use_bbcode) {
parse_bbcode(p_bbcode);
} else { // raw text
clear();
add_text(p_bbcode);
}
}

Since push_*(), pop_*() methods only touch the tag stack, I think it makes sense for clear() to also leave bbcode_text as is and it's the documentation that's wrong.

@Koyper
Copy link
Contributor Author

Koyper commented Dec 11, 2021

In that case I'm wondering why clear() was exposed in the first place? Anything assigned to the bbcode text will call clear() because parse_bbcode also calls clear(). Is there a case where you would want to clear the tag stack but leave behind the bbcode text?

@timothyqiu
Copy link
Member

timothyqiu commented Dec 13, 2021

RichTextLabel is based on a tag stack, so basic stack operations like push_*(), pop_*(), and clear() are available. BBCode text is an easier way to modify the tag stack, but not in reverse. i.e. Setting BBCode updates the tag stack, but changing the tag stack won't affect BBCode. There was a discussion about this somewhere, but I failed to find it now.

The editor help and editor output panel both modify the RichTextLabel with these functions directly instead of BBCode.

@akien-mga akien-mga modified the milestones: 3.4, 3.x Jul 3, 2022
@akien-mga
Copy link
Member

CC @bruvzg. Sounds like this just needs a documentation clarification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants