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 sure cursor visibility is restored after using an IME #6207

Merged
1 commit merged into from
May 27, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented May 26, 2020

Summary of the Pull Request

When using an Input Method Editor in conhost for East Asian languages, the text cursor is temporarily hidden while the characters are being composed. When the composition is complete, the cursor visibility is meant to be restored, but that doesn't always happen if the IME composition is cancelled. This PR makes sure the cursor visibility is always restored, regardless of how the IME is closed.

PR Checklist

Detailed Description of the Pull Request / Additional comments

The original implementation hid the cursor whenever ConsoleImeInfo::WriteCompMessage was called (which could be multiple times in the course of a composition), and then only restored the visibility when ConsoleImeInfo::WriteResultMessage was called. If a composition is cancelled, though, WriteResultMessage would never be called, so the cursor visibility wouldn't be restored.

I've now made the SaveCursorVisibility and RestoreCursorVisibility methods public, so they can instead be called from the ImeStartComposition and ImeEndComposition functions. This makes sure RestoreCursorVisibility is always called, regardless of how the composition ended, and SaveCursorVisibility is only called once at the start of the composition (which isn't essential, but seems cleaner to me).

Validation Steps Performed

I've manually tested opening and closing the IME, both while submitting characters and while cancelling a composition, and in all cases the cursor visibility was correctly restored.

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase labels May 26, 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.

This seems reasonable to me, thanks!

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label May 26, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT May 26, 2020 15:45
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

Hello @carlos-zamora!

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.

@ghost ghost merged commit 8752054 into microsoft:master May 27, 2020
jelster pushed a commit to jelster/terminal that referenced this pull request May 28, 2020
…#6207)

## Summary of the Pull Request

When using an _Input Method Editor_ in conhost for East Asian languages, the text cursor is temporarily hidden while the characters are being composed. When the composition is complete, the cursor visibility is meant to be restored, but that doesn't always happen if the IME composition is cancelled. This PR makes sure the cursor visibility is always restored, regardless of how the IME is closed.

## PR Checklist
* [x] Closes microsoft#810
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.

## Detailed Description of the Pull Request / Additional comments

The original implementation hid the cursor whenever `ConsoleImeInfo::WriteCompMessage` was called (which could be multiple times in the course of a composition), and then only restored the visibility when `ConsoleImeInfo::WriteResultMessage` was called. If a composition is cancelled, though, `WriteResultMessage` would never be called, so the cursor visibility wouldn't be restored.

I've now made the `SaveCursorVisibility` and `RestoreCursorVisibility` methods public, so they can instead be called from the `ImeStartComposition` and `ImeEndComposition` functions. This makes sure `RestoreCursorVisibility` is always called, regardless of how the composition ended, and `SaveCursorVisibility` is only called once at the start of the composition (which isn't essential, but seems cleaner to me).

## Validation Steps Performed

I've manually tested opening and closing the IME, both while submitting characters and while cancelling a composition, and in all cases the cursor visibility was correctly restored.
@j4james j4james deleted the fix-ime-cursor branch June 4, 2020 22:58
@ghost
Copy link

ghost commented Jun 18, 2020

🎉Windows Terminal Preview v1.1.1671.0 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

ghost pushed a commit that referenced this pull request Sep 18, 2020
Some IME implementations do not produce composition strings, and their
users have come to rely on the cursor that conhost traditionally left on
until a composition string showed up. We shouldn't hide the cursor until
we get a string (as opposed to hiding it when composition begins) so as
to not break those IMEs.

Related to #6207.

Fixes MSFT:29219348
DHowett added a commit that referenced this pull request Sep 18, 2020
Some IME implementations do not produce composition strings, and their
users have come to rely on the cursor that conhost traditionally left on
until a composition string showed up. We shouldn't hide the cursor until
we get a string (as opposed to hiding it when composition begins) so as
to not break those IMEs.

Related to #6207.

Fixes MSFT:29219348

(cherry picked from commit ef83aa3)
DHowett added a commit that referenced this pull request Nov 16, 2020
Make sure we don't hide the cursor until the IME starts (#7673)

Some IME implementations do not produce composition strings, and their
users have come to rely on the cursor that conhost traditionally left on
until a composition string showed up. We shouldn't hide the cursor until
we get a string (as opposed to hiding it when composition begins) so as
to not break those IMEs.

Related to GH-6207.

Fixes MSFT-29219348
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-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) 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-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text Indicator Disappears When Using Chinese in Command Prompt
4 participants