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 missing punctuation to the default TextEdit word separators #93656

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

vgezer
Copy link
Contributor

@vgezer vgezer commented Jun 27, 2024

Fixes #93618, regression after #92514

Adds to the default separators the following list: ~!@#$%^&*()-=+[{]}|;:'\",.<>;/?

@vgezer vgezer requested review from a team as code owners June 27, 2024 10:26
@Mickeon Mickeon added this to the 4.3 milestone Jun 27, 2024
Mickeon
Mickeon previously approved these changes Jun 27, 2024
@@ -472,7 +472,7 @@ String TextEdit::Text::get_custom_word_separators() const {
}

String TextEdit::Text::get_default_word_separators() const {
String concat_separators = "´`~$^=+|<>";
String concat_separators = "`~!@#$%^&*()-=+[{]}|;:'\",.<>/?";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe those can be excluded with sub-ranges of the ASCII table?

BTW looking at the list, aren't we missing \ (which would need to be escaped as \\)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe those can be excluded with sub-ranges of the ASCII table?

Do you mean iterating through char-codes instead of hardcoding them?

BTW looking at the list, aren't we missing \ (which would need to be escaped as \)?

Yes! You are right. Will be added as well. Iterating would have prevented that, probably :).

Copy link
Member

@akien-mga akien-mga Jun 27, 2024

Choose a reason for hiding this comment

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

Maybe those can be excluded with sub-ranges of the ASCII table?

Do you mean iterating through char-codes instead of hardcoding them?

Yes, though looking at the table it's not a single block so it might not be worth it.
Maybe we can at least list them in the same order as in the ASCII table so that it's easy to spot which ones are included or left out (in particular _ which from a quick look is the only punctuation from the table that we're not including, as we want to support snake_case identifiers I suppose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in particular _ which from a quick look is the only punctuation from the table that we're not including, as we want to support snake_case identifiers I suppose

_ is intentionally left out, because some may prefer selecting the whole word if they prefer this naming convention. If _ needs to be treated as seperator, one should add it into custom_word_separators field in the settings.

@akien-mga akien-mga requested a review from bruvzg June 27, 2024 12:05
@Mickeon Mickeon dismissed their stale review June 27, 2024 12:23

There's a bit of room for improvement

@akien-mga akien-mga changed the title Add missing punctuation to the default list Add missing punctuation to the default TextEdit word separators Jun 27, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Aside from the doc formatting fix, this looks good to me.

@vgezer
Copy link
Contributor Author

vgezer commented Jun 27, 2024

Aside from the doc formatting fix, this looks good to me.

Thanks! Would it be also possible to check whether the regression is gone? I cannot reproduce the bug; hopefully fixed it.

@akien-mga
Copy link
Member

It seems like this change breaks a unit test for TextEdit:

 ./tests/scene/test_text_edit.h:57:
TEST CASE:  [SceneTree][TextEdit] text entry
  [TextEdit] selection
  [TextEdit] skip selection for next occurrence

./tests/scene/test_text_edit.h:1448: ERROR: CHECK( text_edit->get_caret_line(0) == 1 ) is NOT correct!
  values: CHECK( 2 == 1 )

./tests/scene/test_text_edit.h:1449: ERROR: CHECK( text_edit->get_caret_column(0) == 7 ) is NOT correct!
  values: CHECK( 9 == 7 )

./tests/scene/test_text_edit.h:1455: ERROR: CHECK( text_edit->get_caret_line(0) == 1 ) is NOT correct!
  values: CHECK( 3 == 1 )

./tests/scene/test_text_edit.h:1456: ERROR: CHECK( text_edit->get_caret_column(0) == 7 ) is NOT correct!
  values: CHECK( 5 == 7 )

You can test locally by building with tests=yes (implicit when using dev_build=yes) and running Godot with the --test argument.

@vgezer
Copy link
Contributor Author

vgezer commented Jun 27, 2024

You can test locally by building with tests=yes (implicit when using dev_build=yes) and running Godot with the --test argument.

Thanks. Forgot to disable _ in the default list, which caused one test to fail.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested, I confirm this fixes the linked issue and seems to reintroduce the same expected behavior as in 4.2.2-stable.

@akien-mga akien-mga merged commit a647789 into godotengine:master Jun 28, 2024
18 checks passed
@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.

Inconsistent and Broken Skipping Cursor in Editor - Godot 4.3 Beta 2 Custom
3 participants