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

A minor TSF refactoring #17067

Merged
merged 7 commits into from
Apr 18, 2024
Merged

A minor TSF refactoring #17067

merged 7 commits into from
Apr 18, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 16, 2024

Next in the popular series of minor refactorings:
Out with the old, in with the new!

This PR removes all of the existing TSF code, both for conhost and
Windows Terminal. conhost's TSF implementation was awful:
It allocated an entire text buffer per line of input.
Additionally, its implementation spanned a whopping 40 files and
almost 5000 lines of code. Windows Terminal's implementation was
absolutely fine in comparison, but it was user unfriendly due to
two reasons: Its usage of the CoreTextServices WinRT API indirectly
meant that it used a non-transitory TSF document, which is not the
right choice for a terminal. A TF_SS_TRANSITORY document (-context)
indicates to TSF that it cannot undo a previously completed composition
which is exactly what we need: Once composition has completed we send
the result to the shell and we cannot undo this later on.
The WinRT API does not allow us to use TF_SS_TRANSITORY and so it's
unsuitable for our application. Additionally, the implementation used
XAML to render the composition instead of being part of our text
renderer, which resulted in the text looking weird and hard to read.

The new implementation spans just 8 files and is ~1000 lines which
should make it significantly easier to maintain. The architecture is
not particularly great, but it's certainly better than what we had.
The implementation is almost entirely identical between both conhost
and Windows Terminal and thus they both also behave identical.
It fixes an uncountable number of subtle bugs in the conhost TSF
implementation, as it failed to check for status codes after calls.
It also adds several new features, like support for wavy underlines
(as used by the Japanese IME), dashed underlines (the default for
various languages now, like Vietnamese), colored underlines,
colored foreground/background controlled by the IME, and more!

I have tried to replicate the following issues and have a high
confidence that they're resolved now:
Closes #1304
Closes #3730
Closes #4052
Closes #5007 (as it is not applicable anymore)
Closes #5110
Closes #6186
Closes #6192
Closes #13805
Closes #14349
Closes #14407
Closes #16180

For the following issues I'm not entirely sure if it'll fix it,
but I suspect it's somewhat likely:
#13681
#16305
#16817

Lastly, there's one remaining bug that I don't know how to resolve.
However, that issue also plagues conhost and Windows Terminal
right now, so it's at least not a regression:

  • Press Win+. (emoji picker) and close it
  • Move the window around
  • Press Win+.

This will open the emoji picker at the old window location.
It also occurs when the cursor moves within the window.
While this is super annoying, I could not find a way to fix it.

Validation Steps Performed

  • See the above closed issues
  • Use Vietnamese Telex and type "xin choaf"
    Results in "xin chào" ✅
  • Use the MS Japanese IME and press Alt+`
    Toggles between the last 2 modes ✅
  • Use the MS Japanese IME, type "kyouhaishaheiku", and press Space
    • The text is converted, underlined and the first part is
      doubly underlined ✅
    • Left/Right moves between the 3 segments ✅
    • Home/End moves between start/end ✅
    • Esc puts a wavy line under the current segment ✅
  • Use the Korean IME, type "gksgks"
    This results in "한한" ✅
  • Use the Korean IME, type "gks", and press Right Ctrl
    Opens a popup which allows you to navigate with Arrow/Tab keys ✅

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Area-i18n Internationalization issues (e.g.: non-US input handling doesn't work) Area-Input Related to input processing (key presses, mouse, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Priority-1 A description (P1) Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Apr 16, 2024
@zadjii-msft
Copy link
Member

i dunno though, is fixing 11 issues in one PR enough to be considered "minor"? this is a trivial refactoring at best 😝

@miniksa
Copy link
Member

miniksa commented Apr 17, 2024

You're a champion, @lhecker

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Most of the PR is deleted files and deleting references to the deleted stuff, so that's nice.

The most complicated part is src/tsf/Implementation.cpp, but a good amount of the complexity there is that it's a lot of COM.

That said, looks great!

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
Comment on lines +187 to +190
.columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
};

state.text = text.substr(0, _pData->activeComposition.cursorPos);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
};
state.text = text.substr(0, _pData->activeComposition.cursorPos);
.columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
.text = text.substr(0, _pData->activeComposition.cursorPos),
};

Curious, any particular reason you didn't initialize it here with columnLimit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it so that the two ReplaceText() calls look somewhat symmetric. I felt like it's easier to read/understand that way.

@@ -195,9 +270,6 @@ try
// 2. Paint Rows of Text
_PaintBufferOutput(pEngine);

// 3. Paint overlays that reside above the text buffer
_PaintOverlays(pEngine);

// 4. Paint Selection
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 4. Paint Selection
// 3. Paint Selection

Copy link
Member

Choose a reason for hiding this comment

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

and update the other numbers below

Copy link
Member Author

@lhecker lhecker Apr 18, 2024

Choose a reason for hiding this comment

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

I'd prefer not changing this code right now, because I'd like to remove it in the near-ish term and changing it now would only disturb the git blame until then IMHO.

OpenConsole.sln Show resolved Hide resolved
src/interactivity/win32/windowproc.cpp Show resolved Hide resolved
src/interactivity/win32/windowproc.cpp Show resolved Hide resolved
@@ -1003,9 +998,6 @@ DWORD WINAPI ConsoleInputThreadProcWin32(LPVOID /*lpParameter*/)
// -- END LOAD BEARING CODE
}

// Free all resources used by this thread
DeactivateTextServices();
Copy link
Member

Choose a reason for hiding this comment

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

are there any resources we need to free in the new code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, yes. However, this code is currently effectively unreachable, because we TerminateProcess in the ServiceLocator on shutdown. A better location for shutting down TSF is in the destructor of Window but I figured it's fine if we just don't. After all, we also got the delete renderer calls commented out there.

src/interactivity/win32/windowio.cpp Outdated Show resolved Hide resolved
src/tsf/Implementation.cpp Show resolved Hide resolved
@lhecker lhecker added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit 4e7b63c Apr 18, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/8000-conime branch April 18, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-i18n Internationalization issues (e.g.: non-US input handling doesn't work) Area-Input Related to input processing (key presses, mouse, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
5 participants