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.

Sign up for GitHub

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: Ensures correct IME behavior when the text input area gains or loses focus. #4896

Merged
merged 9 commits into from
Aug 28, 2024
Merged

Fix: Ensures correct IME behavior when the text input area gains or loses focus. #4896

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5961eb8
Update builder.rs
rustbasic Aug 1, 2024
b277d39
Update builder.rs
rustbasic Aug 3, 2024
07e8de2
Update builder.rs
rustbasic Aug 3, 2024
81ef0bf
Update builder.rs
rustbasic Aug 4, 2024
104bef2
Update builder.rs
rustbasic Aug 4, 2024
bb0dfe4
Update builder.rs
rustbasic Aug 27, 2024
87b90f3
Merge branch 'emilk:master' into patch92
rustbasic Aug 27, 2024
6ef2ef5
Update builder.rs
rustbasic Aug 27, 2024
f58742c
Merge branch 'emilk:master' into patch92
rustbasic Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/egui/src/widgets/text_edit/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,10 @@ impl<'t> TextEdit<'t> {
}
}

if state.ime_enabled && response.lost_focus() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldnt you just leave the state.ime_enabled` ? Because if its true its getting set to false and else it is already false, so to me it seems like an unnecessary condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldnt you just leave the state.ime_enabled` ? Because if its true its getting set to false and else it is already false, so to me it seems like an unnecessary condition

When focus is moved elsewhere, you must set ime_enabled to 'false`, otherwise the IME will have problems when focus returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure the idea was the following

if response.lost_focus() {
state.ime_enabled = false;
}

Copy link
Contributor Author

@rustbasic rustbasic Aug 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sure the idea was the following

if response.lost_focus() {
state.ime_enabled = false;
}

Yes, I thought about this too. If ime_enabled is not true, there is no need to check lost_focus() , so there is a difference in speed and readability. Let's wait for emilk's opinion on which one is better.

thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok but if so it would be good to add a comment there anyway

state.ime_enabled = false;
}

state.clone().store(ui.ctx(), id);

if response.changed {
Expand Down
Loading