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

Needs to input twice to insert a single Chinese symbol #3745

Closed
hez2010 opened this issue Nov 27, 2019 · 18 comments · Fixed by #4140
Closed

Needs to input twice to insert a single Chinese symbol #3745

hez2010 opened this issue Nov 27, 2019 · 18 comments · Fixed by #4140
Assignees
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Milestone

Comments

@hez2010
Copy link

hez2010 commented Nov 27, 2019

Environment

Windows build number: 18363.476
Windows Terminal version (if applicable): 0.7.3291.0

Steps to reproduce

  1. switch to Microsoft Chinese Input Method
  2. press shift + / to insert a Chinese

Expected behavior

successfully inserted

Actual behavior

I have to press shift + / twice to insert a single

Same problem with other Chinese symbols, such as , , , and etc.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 27, 2019
@zadjii-msft zadjii-msft added 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 Nov 27, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 27, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Nov 27, 2019
@spindensity
Copy link

Same problem here, need to press the corresponding key twice to input one Chinese punctuation.

Windows terminal 0.7.3291.0

@mkitzan
Copy link
Contributor

mkitzan commented Nov 30, 2019

I found an interesting pattern when repro-ing this issue. The double-input behavior can be repro-ed with any character by performing the following:

  • Set language to Chinese
  • Open new WT instance
  • Enter question mark (this first question mark should print to screen fine)
  • Enter any character, C1 (C1 will not be printed to screen)
  • Enter any character, C2 (C2 will be printed to screen, possibly C1 too depending on its character)

Based off this behavior, it appears that after entering certain characters (like question mark), WT is in a buggy state which on transition out of that state the character is consumed and not printed. I wonder if the issue is in one of the StateMachine classes or TerminalInput.

Interesting to note that this is not a problem with the Japanese language package.

@mkitzan
Copy link
Contributor

mkitzan commented Nov 30, 2019

In TSFInputControl.cpp, the CoreTextEditContext is becoming desynced on the CompositionCompleted event when _inputBuffer and _textBlock are cleared but the NotfiyTextChanged/NotifySelectionChanged fail (or there's a lag) to update the server. Because of this, the caret is not reset to 0,0 on the start of a new composition leading to the out_of_range exception. After catching the exception, CoreTextTextUpdatingResult::Failed is sent which causes the server to finally pick up the update from NotfiyTextChanged/NotifySelectionChanged.

Exception thrown at 0x00007FFB872CA839 in WindowsTerminal.exe: Microsoft C++ exception: std::out_of_range at memory location 0x0000005CE78FE150.
Exception thrown at 0x00007FFB872CA839 in WindowsTerminal.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
C:\GitHub\terminal-fork\src\cascadia\TerminalControl\TSFInputControl.cpp(328)\TerminalControl.dll!00007FFB46C165CD: (caller: 00007FFB46BE1CAD) LogHr(1) tid(42cc) 8000000B The operation attempted to access data outside the valid range
    Msg:[std::exception: invalid string position] 

@mkitzan
Copy link
Contributor

mkitzan commented Dec 1, 2019

The actual call to NotifyTextChanged appears malformed because the first argument should be the modified range ie the entire _inputBuffer range. However, the corrected function call still doesn't update the CoreTextEditContext server. I wonder if NotifyTextChanged can only be called outside of a CoreTextEditContext handler.

@mkitzan
Copy link
Contributor

mkitzan commented Dec 2, 2019

Because CoreTextEditContext is not behaving as expected, could just implement the Caret and Selection ourselves and use the CoreTextEditContext events to receive text and composition start/complete signals. It would be pretty hacky, but I haven't been able to find a clean fix. Maybe someone who has more experience with UWP custom text input would have some insight into a solution.

@zadjii-msft
Copy link
Member

@mkitzan thanks for the investigation! I'm pretty sure that @philnach might be interested in your findings

@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 2, 2019
@lhecker
Copy link
Member

lhecker commented Jan 6, 2020

@zadjii-msft If you change this:

_editContext.NotifyTextChanged(emptyTextRange, 0, emptyTextRange);
_editContext.NotifySelectionChanged(emptyTextRange);

...into this:

_editContext.NotifyFocusLeave();
_editContext.NotifyTextChanged(emptyTextRange, 0, emptyTextRange);
_editContext.NotifyFocusEnter();

...this issue and #3706 (and others?) should be fixed. 🙂 And #3645 is probably void too, because it seems to work even if you comment this out:

// _editContext.NotifyFocusLeave(); TODO GitHub #3645: Enabling causes IME to no longer show up, need to determine if required

Would you like me to submit a PR tomorrow?

@zadjii-msft
Copy link
Member

@lhecker I would love a PR from you 😄

@lhecker
Copy link
Member

lhecker commented Jan 7, 2020

Oh I just noticed that NotifyTextChanged() is being used incorrectly anyways. The first argument is supposed to be the range that's modified. In this case it would be [0, the_previous_buffer_length] instead of [0, 0]. I'll try fixing this later today.

@ghost ghost added the In-PR This issue has a related PR label Jan 8, 2020
@ghost ghost closed this as completed in #4140 Jan 8, 2020
ghost pushed a commit that referenced this issue Jan 8, 2020
The first argument to `NotifyTextChanged` incorrectly was `[0,0]`
instead of the length of the text to be removed from the
`CoreTextEditContext`.

Best source of documentation for `NotifyTextChanged`:
https://docs.microsoft.com/en-us/windows/uwp/design/input/custom-text-input#overriding-text-updates

FYI @DHowett-MSFT (just in case): C++/WinRT uses `winrt::param::hstring`
for string parameters which intelligently borrows strings. As such you
can simply pass a `std::wstring` to most WinRT methods without the need
of having to allocate an intermediate `hstring`. 🙂

## Validation Steps Performed

I followed the reproduction instructions of #3706 and #3745 and ensured
the issue doesn't happen anymore.

Closes #3645
Closes #3706
Closes #3745
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 8, 2020
@zadjii-msft
Copy link
Member

Wait though we're not sure this is fixed, but the others were

@zadjii-msft zadjii-msft reopened this Jan 8, 2020
@ghost
Copy link

ghost commented Jan 14, 2020

🎉This issue was addressed in #4140, which has now been successfully released as Windows Terminal Preview v0.8.10091.0.:tada:

Handy links:

@spindensity
Copy link

The problem is still here.

Windows terminal v0.8.10091.0

@zadjii-msft
Copy link
Member

Welp the bot got confused here.

@lhecker
Copy link
Member

lhecker commented Jan 14, 2020

Sorry for your troubles @zadjii-msft. And I'm sorry for causing this confusion.
I commited my changes as "Fixes/Closes #3745", as I was unable to reproduce this issue with my changes.
It turned out though that this issue simply doesn't happen anymore in an upcoming Windows version - Windows 20H1 - set to be released in spring this year and a version I'm already using on my PC (through the Windows Insiders Program). Older versions still have this issue, which is why it was reopened.

ghost pushed a commit that referenced this issue Feb 26, 2020
## Summary of the Pull Request
Currently, while in IME mode, selections with the Emoji/Kaomoji/Symbol Picker (which is brought up with <kbd>win+.</kbd>) are not displayed until the user starts a new composition. This is due to the fact that we hide the TextBlock when we receive a CompositionCompleted event, and we only show the TextBlock when we receive a CompositionStarted event. Input from the picker does not count as a composition, so we were never showing the text box, even if the symbols were thrown into the inputBuffer. In addition, we weren't receiving CompositionStarted events when we expected to.

We should be showing the TextBlock when we receive _any_ text, so we should make the TextBlock visible inside of `TextUpdatingHandler`. Furthermore, some really helpful discussion in #3745 around wrapping the `NotifyTextChanged` call with a `NotifyFocusLeave` and a `NotifyFocusEnter` allowed the control to much more consistently determine when a CompositionStarted and a CompositionEnded.

I've also went around and replaced casts with saturating casts, and have removed the line that sets the `textBlock.Width()` so that it would automatically set its width. This resolves the issue where while composing a sentence, the textBlock would be too small to contain all the text, so it would be cut off, but the composition is still valid and still able to continue.

## PR Checklist
* [x] Closes #4148
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed

## Validation Steps Performed
Tested picking emojis, kaomojis, and symbols with numerous different languages.
@leonMSFT
Copy link
Contributor

leonMSFT commented Mar 4, 2020

I'd be curious to see if this still repros considering #4688 and #4796 should have alleviated this issue (I think). I personally can't seem to repro it anymore, so I'll ask others to try it out and if this doesn't seem to occur anymore, I'll close the issue!

@zyy1998
Copy link

zyy1998 commented Mar 5, 2020

I'd be curious to see if this still repros considering #4688 and #4796 should have alleviated this issue (I think). I personally can't seem to repro it anymore, so I'll ask others to try it out and if this doesn't seem to occur anymore, I'll close the issue!

Actually when I upgraded my system to 10.0.19564.0 preview version, this issue disappeared.

@leonMSFT leonMSFT self-assigned this Mar 12, 2020
@leonMSFT
Copy link
Contributor

Thanks for the confirmation! I've also checked on one of my machines running an older version of windows and I can repro, but once I'm on a newer build, it doesn't repro anymore. I've also checked with some other members of the team and they can't repro either, so I'll go ahead and close this one.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Mar 12, 2020
@Lemmingh
Copy link

After upgrading Terminal to version 0.10.761.0, the problem also disappears on Windows Version 1909.


Windows build number: 10.0.18363.720
Windows Terminal version: 0.10.761.0

IME: Microsoft Pinyin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants