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

@spindensity commented on GitHub (Nov 28, 2019):

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

Windows terminal 0.7.3291.0

@spindensity commented on GitHub (Nov 28, 2019): Same problem here, need to press the corresponding key twice to input one Chinese punctuation. Windows terminal 0.7.3291.0

claunia commented

@mkitzan commented on GitHub (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 commented on GitHub (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.

claunia commented

@mkitzan commented on GitHub (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 commented on GitHub (Nov 30, 2019): In [`TSFInputControl.cpp`](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalControl/TSFInputControl.cpp), the `CoreTextEditContext` is becoming desynced on the `CompositionCompleted` event when `_inputBuffer` and `_textBlock` are [cleared](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalControl/TSFInputControl.cpp#L218) but the [`NotfiyTextChanged`/`NotifySelectionChanged`](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalControl/TSFInputControl.cpp#L227) 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] ```

claunia commented

@mkitzan commented on GitHub (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 commented on GitHub (Dec 1, 2019): The actual call to [`NotifyTextChanged`](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalControl/TSFInputControl.cpp#L227) 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.

claunia commented

@mkitzan commented on GitHub (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.

@mkitzan commented on GitHub (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.

claunia commented

@zadjii-msft commented on GitHub (Dec 2, 2019):

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

@zadjii-msft commented on GitHub (Dec 2, 2019): @mkitzan thanks for the investigation! I'm pretty sure that @philnach might be interested in your findings

claunia commented

@lhecker commented on GitHub (Jan 6, 2020):

@zadjii-msft If you change this: 4882917499/src/cascadia/TerminalControl/TSFInputControl.cpp (L227-L228)
...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: 4882917499/src/cascadia/TerminalControl/TSFInputControl.cpp (L107)

Would you like me to submit a PR tomorrow?

@lhecker commented on GitHub (Jan 6, 2020): @zadjii-msft If you change this: https://github.com/microsoft/terminal/blob/488291749994822ef54ba5140c7726b6d984116f/src/cascadia/TerminalControl/TSFInputControl.cpp#L227-L228 ...into this: ```cpp _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: https://github.com/microsoft/terminal/blob/488291749994822ef54ba5140c7726b6d984116f/src/cascadia/TerminalControl/TSFInputControl.cpp#L107 Would you like me to submit a PR tomorrow?

claunia commented

@zadjii-msft commented on GitHub (Jan 7, 2020):

@lhecker I would love a PR from you 😄

@zadjii-msft commented on GitHub (Jan 7, 2020): @lhecker I would _love_ a PR from you 😄

claunia commented

@lhecker commented on GitHub (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.

@lhecker commented on GitHub (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.

claunia commented

@zadjii-msft commented on GitHub (Jan 8, 2020):

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

@zadjii-msft commented on GitHub (Jan 8, 2020): Wait though we're not sure this is fixed, but the others were

claunia commented

@ghost commented on GitHub (Jan 14, 2020):

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

Handy links:

@ghost commented on GitHub (Jan 14, 2020): :tada:This issue was addressed in #4140, which has now been successfully released as `Windows Terminal Preview v0.8.10091.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v0.8.10091.0) * [Store Download](https://www.microsoft.com/store/apps/9n0dx20hk701?cid=storebadge&ocid=badge)

claunia commented

@spindensity commented on GitHub (Jan 14, 2020):

The problem is still here.

Windows terminal v0.8.10091.0

@spindensity commented on GitHub (Jan 14, 2020): The problem is still here. Windows terminal v0.8.10091.0

claunia commented

@zadjii-msft commented on GitHub (Jan 14, 2020):

Welp the bot got confused here.

@zadjii-msft commented on GitHub (Jan 14, 2020): Welp the bot got confused here.

claunia commented

@lhecker commented on GitHub (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.

@lhecker commented on GitHub (Jan 14, 2020): Sorry for your troubles @zadjii-msft. And I'm sorry for causing this confusion. I commited [my changes](https://github.com/microsoft/terminal/pull/4140) 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.

claunia commented