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

Fix TextEdit IME issues #87479

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Fix TextEdit IME issues #87479

merged 1 commit into from
Feb 13, 2024

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Jan 22, 2024

IME text is now applied before most actions, so editing text and changing carets won't cause issues.

IME is applied before the right click context menu appears, before using the edit menu for any operation, and when clicking elsewhere in the TextEdit.

Scrolling with the mousewheel is no longer blocked when the IME is open.
Updating the IME will adjust the view to the caret and update the IME window position.

@MewPurPur
Copy link
Contributor

MewPurPur commented Jan 24, 2024

I tested the big PR and found that the issue I deducted in #83795 has been fixed. So this PR likely still fixes it.

scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
}
text.invalidate_cache(get_caret_line(i), get_caret_column(i), true, t, structured_text_parser(st_parser, st_args, t));
}
if (has_ime_text()) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: _update_ime_text already checks has_ime_text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't check for it, it would run the other branch in _update_ime_text() and invalidates the text cache unnecessarily.

scene/gui/text_edit.h Show resolved Hide resolved
@kitbdev kitbdev requested a review from a team as a code owner January 29, 2024 18:07
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.

I cannot speak on whether this is a good bunch of changes or not, but a few thoughts on the docs:

doc/classes/TextEdit.xml Outdated Show resolved Hide resolved
doc/classes/TextEdit.xml Outdated Show resolved Hide resolved
doc/classes/TextEdit.xml Outdated Show resolved Hide resolved
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 overall. Let's get this merged to unblock the next batch of fixes :)

@akien-mga akien-mga merged commit 48e2f43 into godotengine:master Feb 13, 2024
16 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.

6 participants