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 Highlighting, Redacting #35608

Merged
merged 1 commit into from
Jun 20, 2021
Merged

RichTextLabel Highlighting, Redacting #35608

merged 1 commit into from
Jun 20, 2021

Conversation

golfinq
Copy link
Contributor

@golfinq golfinq commented Jan 27, 2020

This pull request addresses godotengine/godot-proposals#367 by adding highlighting and redacting capabilities to RichTextLabel with the tags bgcolor= and fgcolor=

Edit: Removed outdated information

@Chaosus
Copy link
Member

Chaosus commented Jan 27, 2020

Don't forget to merge the commits together as it requested by our workflow, see http://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

@dalexeev
Copy link
Member

Also, the code for parsing BB tags is duplicated in the following file. 😞

godot/editor/editor_help.cpp

Lines 1391 to 1400 in 5db45fb

} else if (tag.begins_with("color=")) {
String col = tag.substr(6, tag.length());
Color color;
if (col.begins_with("#"))
color = Color::html(col);
else if (col == "aqua")
color = Color(0, 1, 1);
else if (col == "black")

@golfinq
Copy link
Contributor Author

golfinq commented Jan 27, 2020

Please note for the PR to work for 3.2; change the parse_color function to below. As the current state of the PR is assuming the changes made to color.h and color.cpp from #35505.

Color RichTextLabel::parse_color(const String &p_color) {
	Color color;

	if (Color::html_is_valid(p_color))
		color = Color::html(p_color);
	else
		color = Color::named(p_color);

	if (color == Color() && p_color != "black" && p_color != "#000000")
		color = get_color("default_color");

	return color;
}

@dalexeev
Copy link
Member

	if (color == Color() && color != "black" && color != "#000000")

In the second and third conditions, instead of color, should there be p_color? 😉

@golfinq
Copy link
Contributor Author

golfinq commented Jun 7, 2020

I changed the selection behavior of the fgcolor tag to change the color of the selected part to the selection color.

@dalexeev
Copy link
Member

dalexeev commented Jan 8, 2021

@golfinq #35505 was finally merged, so you can rebase your PR. 😃

@golfinq
Copy link
Contributor Author

golfinq commented Jan 14, 2021

This pull request is now updated and It should work with all of the characters RichTextLabel is now expected to draw

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Feature looks good to me. We'll need it at some point to improve the display of the built-in editor help (e.g. making inline code stand out more).

Make sure to update the class reference XML to document the newly added methods.

@golfinq golfinq requested review from a team as code owners March 30, 2021 12:34
@golfinq golfinq requested review from Calinou and removed request for a team June 17, 2021 15:15
@golfinq
Copy link
Contributor Author

golfinq commented Jun 17, 2021

I am unsure what I did, but I just wanted to request review, not remove one. Sorry.

@YuriSizov YuriSizov requested review from a team June 17, 2021 15:35
@KoBeWi
Copy link
Member

KoBeWi commented Jun 17, 2021

The background doesn't properly fit to letters:
image
Compare with e.g. Google Docs:
image

Also this doesn't fully resolve the proposal, as the user wanted the "redacted" text to be unselectable. This is fine and doesn't need to be implemented here, but the OP needs to be updated not to auto-close the proposal.

@Calinou
Copy link
Member

Calinou commented Jun 17, 2021

Also this doesn't fully resolve the proposal, as the user wanted the "redacted" text to be unselectable. This is fine and doesn't need to be implemented here, but the OP needs to be updated not to auto-close the proposal.

To draw redacted text that cannot be retrieved by external means, it's likely better to replace the text with a Unicode character (and keep spaces as-is if desired). This ensures screen readers and the like can't access the text (once screen reader support is implemented).

@golfinq
Copy link
Contributor Author

golfinq commented Jun 18, 2021

The background doesn't properly fit to letters

I figured out a solution and now properly fills the entire line.
Image

the user wanted the "redacted" text to be unselectable.

To prevent copy/paste will be more tricky and would probably be best done in a separate pull request. My implementation would be to create a flag in each Glyph object which would tell the text server whether or not to draw the actual characters, but keep track of the spacing for everything else. Then find where Godot communicates with the host OS to fill the clipboard and skip any character which has that flag set. (Assuming that is how it works)

to replace the text with a Unicode character

Would this work with fonts that do not support unicode?

There are definitely other ways though, and as mentioned I think is outside of the scope of this pull request.

@Calinou
Copy link
Member

Calinou commented Jun 18, 2021

Would this work with fonts that do not support unicode?

Yes, by defining a fallback font that contains the character you need. It will be used if the main font doesn't contain the glyph in question.

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.

There's an issue with this approach (drawing highlights for each glyph individually) if you are using font/script with the overlapping glyphs.

Screenshot 2021-06-19 at 10 49 14

On the screenshot, semi transparent bgcolor is covering part of the text.

For this reason, text selection is fully drawn before drawing any text(see lines 940-953), same should be done for highlights:

  1. Draw selection and bgcolor rectangles.
  2. Draw text outlines.
  3. Draw main text.
  4. Draw fgcolor rectangles.

Text server have dedicated for this specific purpose, which return non overlapping highlight/selection ranges for the portion of text:

Vector<Vector2> range = TS->shaped_text_get_selection(rid, start_position, end_position);

@golfinq
Copy link
Contributor Author

golfinq commented Jun 19, 2021

I believe it is now working as expected. Here is my test text

[fgcolor=red]afdasagrffag[/fgcolor]asdf
Lorem ipsum dolor sit amet,
 [bgcolor=#0000FF22] الحدود وصافرات, مع و [/bgcolor] aing elit, sed
[bgcolor=#00000044]do eiu smod[bgcolor=black] वंबर जैसी अत्यंत क्ष[/bgcolor] incididunt[/bgcolor] 
rr
ff [bgcolor=red]将エ管話ツナfffgg ルヒ社[/bgcolor] ff

test [bgcolor=black]adsfg[bgcolor=yellow]afg[/bgcolor]fdasfdasfj[/bgcolor]

Here is the output
image

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.

Using over glyph array won't work if there are writing direction changes in the middle of highlight, using character range instead should work.

scene/gui/rich_text_label.h Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
@golfinq
Copy link
Contributor Author

golfinq commented Jun 19, 2021

Using over glyph array won't work if there are writing direction changes in the middle of highlight, using character range instead should work.

It worked! For reference here is my test text where I believe exactly what you describe happens:

ipsum dolor
[fgcolor=red]afdasagrffag[/fgcolor]
Lorem ipsum dolor sit amet,
[bgcolor=#0000FF22] الحدود وصافرات, مع وkkl ghjt[/bgcolor] aing elit, sed
[bgcolor=#00000044]do eiu smod[bgcolor=black] वंबर जैसी अत्यंत क्ष[/bgcolor] incididunt[/bgcolor][fgcolor=black]quickjjkkgG[/fgcolor]
     [bgcolor=black]across lines
rr[/bgcolor]jk
ff [bgcolor=red]将エ管話ツナfffgg ルヒ社[/bgcolor] ff

test [bgcolor=black]adsfg[bgcolor=yellow]afg[/bgcolor]fdasfdasfj[/bgcolor]

and here is the result

image

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.

CI log:

scene/gui/rich_text_label.cpp:4187:32: error: 'INT_MAX' was not declared in this scope
 4187 |  Vector2i fbg_index = Vector2i(INT_MAX, INT_MIN);
      |                                ^~~~~~~
scene/gui/rich_text_label.cpp:46:1: note: 'INT_MAX' is defined in header '<climits>'; did you forget to '#include <climits>'?
   45 | #include "editor/editor_scale.h"
  +++ |+#include <climits>
   46 | #endif
scene/gui/rich_text_label.cpp:4187:41: error: 'INT_MIN' was not declared in this scope
 4187 |  Vector2i fbg_index = Vector2i(INT_MAX, INT_MIN);
      |                                         ^~~~~~~
scene/gui/rich_text_label.cpp:4187:41: note: 'INT_MIN' is defined in header '<climits>'; did you forget to '#include <climits>'?
scene/gui/rich_text_label.cpp:4218:13: error: declaration of 'i' shadows a previous local [-Werror=shadow=local]
 4218 |    for (int i = 0; i < sel.size(); i++) {
      |             ^
scene/gui/rich_text_label.cpp:4191:11: note: shadowed declaration is here
 4191 |  for (int i = start; i < end; i++) {
      |           ^

scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@PseudoC0de
Copy link

Any chance to have this back-ported to 3.x? It would be very useful to have this available while 4.x releases and catches up in stability/production-usability.

@Calinou
Copy link
Member

Calinou commented Apr 20, 2022

Any chance to have this back-ported to 3.x? It would be very useful to have this available while 4.x releases and catches up in stability/production-usability.

Backporting efforts are welcome, but this requires an entirely different implementation in 3.x since RichTextLabel was rewritten from scratch in master.

@golfinq
Copy link
Contributor Author

golfinq commented Apr 21, 2022

This was first written it was in 3.x, but was creating the box per glyph; which lead to issues with international alphabets and wasn't great at identifying the proper offsets for the line it was filling. Subsequently, the PR was entirely rewritten using the master branch glyph position handling to identify the height and where to start and stop drawing the boxes. It has been a while since I've looked at the 3.x source code; but it might be possible to borrow the underline positioning algorithm to backport to 3.x as the tag stack filling and identifying mostly stayed the same. Unfortunately, I am not in a position to volunteer code at this time, but that would be the path I would pursue.

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.

8 participants