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

Enable custom separators to treat different characters as words #92514

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

vgezer
Copy link
Contributor

@vgezer vgezer commented May 29, 2024

Implements proposal written here: godotengine/godot-proposals#9837, including comment godotengine/godot-proposals#9837 (comment)

Adds two options in Editor Settings to treat custom defined words as word delimiters/separators. If the first option is enabled, it uses the list below to stop caret on these characters to ease subword movement as in VSCode and some other editors.

image

Uses #46721, fixes godotengine/godot-proposals#5731 and closes #69254 and closes #91964

The functionality is disabled by default.

@AThousandShips
Copy link
Member

Is this meant to implement:

Or is more required for that?

@@ -2340,6 +2340,9 @@ void TextEdit::_move_caret_left(bool p_select, bool p_move_by_word) {
set_caret_line(get_caret_line(i) - 1, false, true, -1, i);
set_caret_column(text[get_caret_line(i)].length(), i == 0, i);
} else {
if (is_use_custom_separators_enabled()) { //&& get_word_separators().size() != 0) {
TS->shaped_text_set_custom_punctuation(text.get_line_data(get_caret_line(i))->get_rid(), get_word_separators());
Copy link
Member

Choose a reason for hiding this comment

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

This should be set when line RIDs are initialized, if it's used here, it will do unnecessary full text reprocessing and won't apply custom separators to the line breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions where it may best fit?

Copy link
Member

Choose a reason for hiding this comment

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

In the TextEdit::Text::invalidate_cache and TextEdit::Text::invalidate_all_lines() (see how brk_flags are handled, for example).

  • Add set_word_separators implementation to the TextEdit::Text.
  • Add TS->shaped_text_set_custom_punctuation call to the Text::invalidate_*.
  • Call text.set_word_separators and text.invalidate_all_lines from the set_use_custom_separators.

Copy link
Contributor Author

@vgezer vgezer May 30, 2024

Choose a reason for hiding this comment

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

Great tip thanks! However, now I caused a bug I guess:

1) If we change the enable/disable custom separators flag
2) change file
3) close using X without saving the file
4) clicking on save & quit option

does not save the file.

If I do not touch the status of the custom separators flag and perform 2 3 4, it does save the file... Any help how I caused this?

Copy link
Contributor Author

@vgezer vgezer Jun 2, 2024

Choose a reason for hiding this comment

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

Apparently this bug was already there. I created an issue on godot for this.

#92683

@vgezer
Copy link
Contributor Author

vgezer commented May 29, 2024

Is this meant to implement:

Or is more required for that?

Maybe 90 % is done. It also needs camel case subword navigation, which I will look at later after this is merged :)

doc/classes/EditorSettings.xml Outdated Show resolved Hide resolved
doc/classes/EditorSettings.xml Outdated Show resolved Hide resolved
@vgezer vgezer force-pushed the addWordSeparators branch 3 times, most recently from a9882bf to b76a90a Compare May 29, 2024 13:54
@vgezer vgezer marked this pull request as draft May 30, 2024 18:58
@vgezer vgezer marked this pull request as ready for review June 4, 2024 09:46
@vgezer vgezer force-pushed the addWordSeparators branch 3 times, most recently from a0e82a2 to 677aef7 Compare June 5, 2024 09:16
@AThousandShips
Copy link
Member

You used a merge commit to update your branch, please use rebase in the future instead, see here, this grabbed an unrelated commit into the history, you need to remove this

To do this do this:

git reset --hard 195180b
git rebase -i master
git push --force

You will need to make the changes you did after you did the merge commit again

@vgezer
Copy link
Contributor Author

vgezer commented Jun 5, 2024

You used a merge commit to update your branch, please use rebase in the future instead, see here, this grabbed an unrelated commit into the history, you need to remove this.

Sorry, I was doing it before pushing it, but accidentally pushed before squishing them.

@vgezer
Copy link
Contributor Author

vgezer commented Jun 5, 2024

Did you squash instead of resetting? You need to do:

git rebase -i master 

And replace pick with drop before the invalid commit

I did fixup instead of squash, but that was indeed the problem! Drop fixed the issue. Thanks :)

scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
doc/classes/TextEdit.xml Outdated Show resolved Hide resolved
editor/editor_settings.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.h Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.h Outdated Show resolved Hide resolved
scene/gui/text_edit.h Outdated Show resolved Hide resolved
scene/gui/text_edit.h Outdated Show resolved Hide resolved
scene/gui/text_edit.h Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Show resolved Hide resolved
@AThousandShips
Copy link
Member

Once you've applied the changes (in the future please use the batch suggestions option to make all in one change) please squash your commits into one, see here

@vgezer
Copy link
Contributor Author

vgezer commented Jun 11, 2024

Once you've applied the changes (in the future please use the batch suggestions option to make all in one change) please squash your commits into one, see here

Thanks for the reviews! Sorry, my first PR and wanted to squash them in local after applying them all.

@vgezer vgezer force-pushed the addWordSeparators branch 2 times, most recently from 3c27a10 to 8a86525 Compare June 11, 2024 23:23
doc/classes/TextEdit.xml Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
@vgezer vgezer force-pushed the addWordSeparators branch 2 times, most recently from 4918799 to 7f77984 Compare June 13, 2024 09:15
@vgezer
Copy link
Contributor Author

vgezer commented Jun 23, 2024

Some cleanup and added this functionality to the inspector as well, to be used in CodeEditor and TextEditor nodes.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jun 25, 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.

Looks good to me.

This is a new feature but it also fixes a recent regression, so I'll merge this for 4.3 despite the feature freeze.

@akien-mga akien-mga merged commit b63df07 into godotengine:master Jun 25, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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