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

Add select, get_character_rect and hit_test to RichTextLabel #84715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skysphr
Copy link
Contributor

@skysphr skysphr commented Nov 10, 2023

This adds three functions to RichTextLabel, addressing godotengine/godot-proposals#5517, godotengine/godot-proposals#5518, and godotengine/godot-proposals#5519, along with the related godotengine/godot-proposals#1559.

This should make it possible to create a RichTextEditor without further extending the engine, which I will attempt to exemplify in the following days.

I used the name hit_test in order to be consistent with TextServer, TextLine and TextParagraph's nomenclature, although I would have personally picked a more descriptive name such as get_character_at_position, which is similar to what was suggested in the linked proposal.

Production edit:

@skysphr skysphr requested review from a team as code owners November 10, 2023 14:19
@KoBeWi KoBeWi added this to the 4.x milestone Nov 10, 2023
@skysphr skysphr force-pushed the richtextlabel-functions branch 2 times, most recently from 14bd860 to 8407766 Compare November 10, 2023 14:27
@skysphr
Copy link
Contributor Author

skysphr commented Nov 20, 2023

As it turns out, my original implementation for this was rather terrible. Several tests and fixes later I believe I finally have it corrected. Apologies for submitting poor code.

@skysphr skysphr force-pushed the richtextlabel-functions branch from 8407766 to 82fff6c Compare November 20, 2023 13:27
@skysphr skysphr force-pushed the richtextlabel-functions branch from 82fff6c to 8477434 Compare November 20, 2023 13:46
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

This looks desirable.

As it turns out, my original implementation for this was rather terrible. Several tests and fixes later I believe I finally have it corrected. Apologies for submitting poor code.

It's okay we're all collectively doomed to poor coding practices.

int line = -1;
int char_index = -1;

// Find paragraph
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Find paragraph
// Find paragraph.

} break;
}

// Handle tables
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Handle tables
// Handle tables.

Item *c_item = nullptr;
int c_line = 0;
int c_index = 0;
bool outside;
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit may be good here

Suggested change
bool outside;
bool outside = false;

int c_index = 0;
bool outside;

const_cast<RichTextLabel *>(this)->_find_click(main, coords, &c_frame, &c_line, &c_item, &c_index, &outside, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is exceptionally off about this line but I can't quite put my finger on it.

<return type="int" />
<param index="0" name="coords" type="Vector2" />
<description>
Returns caret character offset at the specified coordinates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it means this?
I believe it's a good idea to say "index" here, even if it is often called "position" in Strings. It's reasonably confusing in 2D space where position is a Vector2.

Suggested change
Returns caret character offset at the specified coordinates.
Returns the character index at the given position in local space.

Comment on lines +88 to +89
Returns the bounding box of the character position provided.
[b]Note:[/b] If [member threaded] is enabled, this method returns a value for the loaded part of the document. Use [method is_ready] or [signal finished] to determine whether document is fully loaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Returns the bounding box of the character position provided.
[b]Note:[/b] If [member threaded] is enabled, this method returns a value for the loaded part of the document. Use [method is_ready] or [signal finished] to determine whether document is fully loaded.
Returns the bounding box for the character at the given index.
[b]Note:[/b] If [member threaded] is set to [code]true[/code], this method returns a value for the loaded part of the document. If you need to make sure the document is fully loaded, use [method is_ready] or the [signal finished] signal.

Comment on lines +521 to +522
Selects characters between [param from] and [param to].
If [member selection_enabled] is [code]false[/code], no selection will occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Selects characters between [param from] and [param to].
If [member selection_enabled] is [code]false[/code], no selection will occur.
Selects the characters between the [param from] and [param to] positions. Does nothing if [member selection_enabled] is [code]false[/code].

@mthwJsmith
Copy link

mthwJsmith commented May 23, 2024

Not sure if it is intended, but the first character is both at position 1 and 0 from my testing. 0 prints the same bounding box as 1. So in "hello", get_character_rect(0) and get_character_rect(1) both print the bounding box of the "h".

It also doesnt return the appropriate bounding boxes if [dropcap] or [font-size=] is used to change certain words, it ignores that these have changed the characters size. Will this be fixed by #92461 ?

edit: also misclicked approve changes and not sure how to undo that, sorry about that.

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