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 bbcode_text from RichTextLabel #39148

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Conversation

HaSa1002
Copy link
Contributor

@HaSa1002 HaSa1002 commented May 29, 2020

This simplyfies the text setting and resolves some bugs as the internal data structure is the same.

It's a draft, since the points below must be discussed first (at least 1 to 3)

Things to discuss:

  1. Make add_text, set_text, get_text private (hide them in their current form)?
  2. Rename set_bbcode, get_bbcode and append_bbcode to set_text, get_text and add/append_text?
  3. Rename bbcode_enabled?
  4. How to parse old scene files (since we should put the bbcode_text into the text, when bbcode_enabled is true)?

fixes #36711
fixes #35553 and therefore supersedes #36037 if merged
fixes partly #18413

#35368 seems related acording to the title, but issue itself is unclear

We could alternativly make the RichTextLabel a base class for multiple parsers and reimplement a BBCodeLabel or go in the direction of godotengine/godot-proposals#826

@KoBeWi
Copy link
Member

KoBeWi commented Jun 2, 2020

Current get_text() is actually useful, because it returns the actual text, without bbcodes. This possibility should be preserved somehow.

@HaSa1002
Copy link
Contributor Author

HaSa1002 commented Jun 3, 2020

I thought about having get_parsed_text, or rename the the bbcode text to something like get_raw_text

@HaSa1002 HaSa1002 force-pushed the rtl branch 3 times, most recently from 980c729 to 896c9fd Compare June 22, 2020 22:53
@HaSa1002 HaSa1002 marked this pull request as ready for review June 22, 2020 22:54
@HaSa1002 HaSa1002 requested a review from a team as a code owner June 22, 2020 22:54
@HaSa1002
Copy link
Contributor Author

Merged set_text
Renamed

  • append_bbcode -> append_text
  • get_bbcode -> get_raw_text
  • set_bbcode -> set_text

text property is now set_text and get_raw_text. Maybe a change to get_text and rename of get_text to get_parsed_text is desired for consistency.

@HaSa1002
Copy link
Contributor Author

HaSa1002 commented Apr 2, 2021

rebased and tested.
I don't like having made the text property to set_textand get_raw_text, but I don't know how to phrase get_text_but_without_bbcode. Any ideas?

@KoBeWi
Copy link
Member

KoBeWi commented May 9, 2021

The _validate_property() can be removed.

As for the naming, getter for text should be called get_text(). I feel like the get_text() and get_raw_text() would be fine swapped 🤔 Other names for no-BBCode could be get_clean_text(), get_final_text(), get_unformatted_text() or get_parsed_text().

Also the internal variable bbcode could be renamed to text, because it introduces some confusion.

@HaSa1002
Copy link
Contributor Author

The _validate_property() can be removed.

done.

As for the naming, getter for text should be called get_text(). I feel like the get_text() and get_raw_text() would be fine swapped 🤔 Other names for no-BBCode could be get_clean_text(), get_final_text(), get_unformatted_text() or get_parsed_text().

I renamed get_raw_text() to get_parsed_text(). However, get_text() returns the raw text, if bbcode_enabled is true and the parsed text if not. Is that desired? It looks like a hack for the text <--> bbcode_text ambigity. I deleted that.

Also the internal variable bbcode could be renamed to text, because it introduces some confusion.

done.

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.

Looks alright now.

@akien-mga akien-mga requested a review from a team May 20, 2021 11:32
@reduz
Copy link
Member

reduz commented Aug 30, 2021

Is this the final resolution about this that everyone agrees to? I remember too many discussions on this topic at this point.

@reduz
Copy link
Member

reduz commented Aug 30, 2021

Ok I got confirmation that this is the case, so feel free to rebase and merge.

@HaSa1002
Copy link
Contributor Author

will do

@HaSa1002
Copy link
Contributor Author

HaSa1002 commented Sep 5, 2021

Rebased. One thing to remember: This will erase all text from RTLs as they are not saved in the text property. Can we add a property upgrade hint somehow?

@akien-mga
Copy link
Member

Rebased. One thing to remember: This will erase all text from RTLs as they are not saved in the text property. Can we add a property upgrade hint somehow?

Yes, see e.g. this:

#ifndef DISABLE_DEPRECATED
// Kept for compatibility from 3.x to 4.0.
bool Environment::_set(const StringName &p_name, const Variant &p_value) {
if (p_name == "background_sky") {
set_sky(p_value);
return true;
} else if (p_name == "background_sky_custom_fov") {
set_sky_custom_fov(p_value);
return true;
} else if (p_name == "background_sky_orientation") {
Vector3 euler = p_value.operator Basis().get_euler();
set_sky_rotation(euler);
return true;
} else {
return false;
}
}
#endif

Also renames:
 - append_bbcode -> append_text
 - get_bbcode -> get_text
 - set_bbcode -> set_text
 - get_text -> get_parsed_text

Property text is:
set_text
get_text
@HaSa1002
Copy link
Contributor Author

Rebased. One thing to remember: This will erase all text from RTLs as they are not saved in the text property. Can we add a property upgrade hint somehow?

Yes, see e.g. this:

#ifndef DISABLE_DEPRECATED
// Kept for compatibility from 3.x to 4.0.
bool Environment::_set(const StringName &p_name, const Variant &p_value) {
if (p_name == "background_sky") {
set_sky(p_value);
return true;
} else if (p_name == "background_sky_custom_fov") {
set_sky_custom_fov(p_value);
return true;
} else if (p_name == "background_sky_orientation") {
Vector3 euler = p_value.operator Basis().get_euler();
set_sky_rotation(euler);
return true;
} else {
return false;
}
}
#endif

Thank you. Done and rebased

@akien-mga akien-mga merged commit 60d67f9 into godotengine:master Sep 16, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Text problem Bb Code Enabled / disabled RichTextLabel's text is not copied on duplication
5 participants