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

Make Korean IME input more consistent #4796

Merged
14 commits merged into from
Mar 4, 2020
Merged

Make Korean IME input more consistent #4796

14 commits merged into from
Mar 4, 2020

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Mar 4, 2020

Summary of the Pull Request

Korean IME was not working correctly due to way we were clearing the input buffer inside of TSFInputControl. We wanted to clear our input buffer and tell TSF to clear its input buffer as well when we receive a CompositionCompleted event. This works fine in some IME languages such as Chinese and Japanese. However, Korean IME composes characters differently in such a way where we can't tell TSF to clear their buffer during a CompositionCompleted event because it would clear the character that triggered the CompositionCompleted event in the first place.

The solution in this PR is to keep our _inputBuffer intact until the user presses Enter or Esc, in which case we clear our buffer and the TSF buffer. I've chosen these two keys because it seems to make sense to clear the buffer after text is sent to the terminal with Enter, and Esc usually means to cancel a current composition anyway.

This means we need to keep track of our last known "Composition Start Point", which is represented by _activeTextStart. Whenever we complete a composition, we'll send the portion of the input buffer between _activeTextStart and the end of the input buffer to the terminal. Then, we'll update _activeTextStart to be the end of the input buffer so that the next time we send text to the terminal, we'll only send the portion of our buffer that's "active".

PR Checklist

Validation Steps Performed

Manual testing with Chinese, Japanese, and Korean IME.

@leonMSFT leonMSFT added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Mar 4, 2020
@leonMSFT leonMSFT added this to the Terminal v1.0 milestone Mar 4, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Sure looks good to me.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 4, 2020
@ghost ghost requested review from miniksa, carlos-zamora and DHowett-MSFT March 4, 2020 17:56
@DHowett-MSFT
Copy link
Contributor

@philnach if you've got a couple minutes, do you have any thoughts on this?

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Yep, this is fine.

@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 4, 2020
@ghost
Copy link

ghost commented Mar 4, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett-MSFT
Copy link
Contributor

I've kicked just the x86 build to re-run; stand by.

@ghost ghost merged commit c6879d7 into master Mar 4, 2020
@ghost ghost deleted the dev/lelian/koreaninput branch March 4, 2020 20:01
@DHowett-MSFT
Copy link
Contributor

I believe this made emoji input do a weird thing: it only shows the most recently entered letter now

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Korean IME does not work as expected
4 participants