Merge the TerminalDispatch and AdaptDispatch classes by j4james · Pull Request #13024 · microsoft/terminal

j4james · 2022-05-02T22:07:22Z

Summary of the Pull Request

This PR replaces the TerminalDispatch class with the AdaptDispatch class from conhost, so we're no longer duplicating the VT functionality in two places. It also gives us a more complete VT implementation on the Terminal side, so it should work better in pass-through mode.

References

This is essentially part two of PR #12703.

PR Checklist

Closes De-duplicate code for TerminalDispatch and AdaptDispatch #3849
CLA signed.
Tests added/passed
Documentation updated.
Schema updated.
I've discussed this with core contributors already. Issue number where discussion took place: Further simplification of the ConGetSet interface #12662

Detailed Description of the Pull Request / Additional comments

The first thing was to give the ConGetSet interface a new name, since it's now no longer specific to conhost. I went with ITerminalApi, since that was the equivalent interface on the terminal side, and it still seemed like a generic enough name. I also changed the way the api is managed by the AdaptDispatch class, so it's now stored as a reference rather than a unique_ptr, which more closely matches the way the TerminalDispatch class worked.

I then had to make sure that AdaptDispatch actually included all of the functionality currently in TerminalDispatch. That meant copying across the code for bracketed paste mode, the copy to clipboard operation, and the various ConEmu OSC operations. This also required a few new methods to the ConGetSet/ITerminalApi interface, but for now these are just stubs in conhost.

Then there were a few thing in the api interface that needed cleaning up. The ReparentWindow method doesn't belong there, so I've moved that into PtySignalInputThread class. And the WriteInput method was too low-level for the Terminal requirements, so I've replaced that with a ReturnResponse method which takes a wstring_view.

It was then a matter of getting the Terminal class to implement all the methods in the new ITerminalApi interface that it didn't already have. This was mostly mapping to existing functionality, but there are still a number of methods that I've had to leave as stubs for now. However, what we have is still good enough that I could then nuke the TerminalDispatch class from the Terminal code and replace it with AdaptDispatch.

One oddity that came up in testing, though, was the AdaptDispatch implementation of EraseAll would push a blank line into the scrollback when called on an empty buffer, whereas the previous terminal implementation did not. That caused problems for the conpty connection, because one of the first things it does on startup is send an ED 2 sequence. I've now updated the AdaptDispatch implementation to match the behavior of the terminal implementation in that regard.

Another problem was that the terminal implementation of the color table commands had special handling for the background color to notify the application window that it needed to repaint the background. I didn't want to have to push the color table operations through the ITerminalApi interface, so I've instead moved the handling of the background update into the renderer, initiated by a flag on the TriggerRefreshAll method.

Validation Steps Performed

Surprisingly this PR didn't require a lot of changes to get the unit tests working again. There were just a few methods used from the original ITerminalApi that have now been removed, and which needed an equivalent replacement. Also the updated behavior of the EraseAll method in conhost resulted in a change to the expected cursor position in one of the screen buffer tests.

In terms of manual testing, I've tried out all the different shells in Windows Terminal to make sure there wasn't anything obviously wrong. And I've run a bunch of the tests from vttest to try and get a wider coverage of the VT functionality, and confirmed everything still works at least as well as it used to. I've also run some of my own tests to verify the operations that had to be copied from TerminalDispatch to AdaptDispatch.

DHowett · 2022-05-02T22:21:49Z

i am so excited

j4james · 2022-05-03T01:58:16Z

i am so excited

Yeah, me too. But be aware that the terminal side isn't fully functional as a standalone VT implementation yet. I think the two big things outstanding now are the _AdjustCursorPosition and _WriteBuffer implementations, which should ideally be unified with conhost. We're getting close though.

lhecker · 2022-05-03T11:54:27Z

We should merge this PR before we get to #13025.

lhecker

This PR was surprisingly easy to review! Only TerminalApi.cpp was a bit more complex...

src/terminal/adapter/adaptDispatch.cpp

src/cascadia/TerminalCore/Terminal.cpp

DHowett

So, I must admit that I reviewed this while it was still in draft . . . and I am still totally ready to sign off. I'll keep up to date on individual commits as they come in.

If anything, I've gotten more excited about this change. Passthrough mode is going to be pretty fun, sure, but this is going to unfetter us in a bunch of ways:

I'm going to yeet TerminalAzBridge into the sun once we fill out some of those TODOs in Terminal's TerminalApi
In theory, we could work on DRCS and Sixel support without having to figure out how they transit ConPTY up front, which will let us parallelize that work
Eventually, we may want to split the terminal/adapter directory into terminal/adapter (the shared bits) and host/*adapter* (the conhost-specific bits, like InteractDispatch, that depend on GCI and ServiceLocator)

src/terminal/adapter/ut_adapter/adapterTest.cpp

j4james · 2022-05-03T20:30:23Z

Technically it's ready for review - I don't have plans to add more code - I'm just still doing some testing, and I want to add some copy for the validation section of the commit message.

And yeah Sixel is probably my number one reason for doing this. I've been putting off committing something because I didn't want to add yet another feature that only worked in conhost, but if I can get it to work in Terminal with the passthrough mode (even if imperfectly), I'd feel a lot better about it.

…npty.

DHowett

Knocked it out of the park, as always. Thank you, @j4james

zadjii-msft · 2022-05-04T10:54:37Z

Meh, I was 40/42 on this so far, but I'm too excited. I didn't find anything so far. I trust the other guys, so: @msftbot merge this in 1 minute

ghost · 2022-05-04T11:13:30Z

Hello @zadjii-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.

## Summary of the Pull Request When `TerminalDispatch` was merged with `AdaptDispatch` in PR #13024, that broke the Terminal's `EraseAll` operation in the alt buffer. The problem was that the `EraseAll` implementation makes a call to `SetViewportPosition` which wasn't taking the alt buffer into account, and thus modified the main viewport instead. This PR corrects that mistake. If we're in the alt buffer, the `SetViewportPosition` method now does nothing, since the alt buffer viewport should always be at 0,0. ## References This was a regression introduced in PR #13024. ## PR Checklist * [x] Closes #13038 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #13038 ## Validation Steps Performed I've confirmed that the test case reported in issue #13038 is no longer failing. I've also made sure the `ED 2` and `ED 3` sequences are still working correctly in the main buffer.

ghost · 2022-05-24T17:03:58Z

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

Handy links:

In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore. This has been widely regarded as a good move. Now, you might rightly be wondering: why didn't compilation immediately fail? Well. It turns out that there were _two_ copies of `GetCursorPosition`. One for `const Terminal`, and one for `Terminal`. This is important. `Terminal::GetCursorPosition()` returned the cursor position relative to the viewport. `Terminal::GetCursorPosition() const`, however, returns the cursor position in absolute. We removed the non-`const` one. Fortunately, thanks to the lookup rules for `const`-qualified members, this didn't matter. Code that called `GetCursorPosition()` still called `GetCursorPosition()`, and everything was fine. Except that part about the relative coordinates. That was not fine. The TSF control is the _only_ consumer of `ControlCore.CursorPosition`, and that was the _only_ consumer of relative-`GetCursorPosition()`. This commit restores equilibrium by introducing a new `GetViewportRelativeCursorPosition()` member to `Terminal` and switching over the only consumer of relative cursor position to use it. Closes #13769.

In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore. This has been widely regarded as a good move. Now, you might rightly be wondering: why didn't compilation immediately fail? Well. It turns out that there were _two_ copies of `GetCursorPosition`. One for `const Terminal`, and one for `Terminal`. This is important. `Terminal::GetCursorPosition()` returned the cursor position relative to the viewport. `Terminal::GetCursorPosition() const`, however, returns the cursor position in absolute. We removed the non-`const` one. Fortunately, thanks to the lookup rules for `const`-qualified members, this didn't matter. Code that called `GetCursorPosition()` still called `GetCursorPosition()`, and everything was fine. Except that part about the relative coordinates. That was not fine. The TSF control is the _only_ consumer of `ControlCore.CursorPosition`, and that was the _only_ consumer of relative-`GetCursorPosition()`. This commit restores equilibrium by introducing a new `GetViewportRelativeCursorPosition()` member to `Terminal` and switching over the only consumer of relative cursor position to use it. Closes #13769. Backport of #13785.

In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore. This has been widely regarded as a good move. Now, you might rightly be wondering: why didn't compilation immediately fail? Well. It turns out that there were _two_ copies of `GetCursorPosition`. One for `const Terminal`, and one for `Terminal`. This is important. `Terminal::GetCursorPosition()` returned the cursor position relative to the viewport. `Terminal::GetCursorPosition() const`, however, returns the cursor position in absolute. We removed the non-`const` one. Fortunately, thanks to the lookup rules for `const`-qualified members, this didn't matter. Code that called `GetCursorPosition()` still called `GetCursorPosition()`, and everything was fine. Except that part about the relative coordinates. That was not fine. The TSF control is the _only_ consumer of `ControlCore.CursorPosition`, and that was the _only_ consumer of relative-`GetCursorPosition()`. This commit restores equilibrium by introducing a new `GetViewportRelativeCursorPosition()` member to `Terminal` and switching over the only consumer of relative cursor position to use it. Closes #13769.

In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore. This has been widely regarded as a good move. Now, you might rightly be wondering: why didn't compilation immediately fail? Well. It turns out that there were _two_ copies of `GetCursorPosition`. One for `const Terminal`, and one for `Terminal`. This is important. `Terminal::GetCursorPosition()` returned the cursor position relative to the viewport. `Terminal::GetCursorPosition() const`, however, returns the cursor position in absolute. We removed the non-`const` one. Fortunately, thanks to the lookup rules for `const`-qualified members, this didn't matter. Code that called `GetCursorPosition()` still called `GetCursorPosition()`, and everything was fine. Except that part about the relative coordinates. That was not fine. The TSF control is the _only_ consumer of `ControlCore.CursorPosition`, and that was the _only_ consumer of relative-`GetCursorPosition()`. This commit restores equilibrium by introducing a new `GetViewportRelativeCursorPosition()` member to `Terminal` and switching over the only consumer of relative cursor position to use it. Closes #13769. (cherry picked from commit 2dedc9a) Service-Card-Id: 85131461 Service-Version: 1.15

DHowett mentioned this pull request May 2, 2022

Move AzureConnection back to terminal VT once terminal's VT adapter is robust #4661

Closed

j4james added 12 commits May 3, 2022 00:31

Pass ConGetSet to AdaptDispatch by reference.

99b8ae4

Make api a value field in InteractDispatch and PtySignalInputThread.

eb0a2e8

Rename ConGetSet interface to ITerminalApi.

708c56d

Add conhost stubs for Terminal-only APIs.

d910031

Move ReparentWindow implementation into PtySignalInputThread.

63515d3

Replace WriteInput with ReturnResponse.

a75aa49

Get Terminal to implement VirtualTerminal::ITerminalApi.

6326593

Replace TerminalDispatch with AdaptDispatch.

6a80e72

Handle EraseAll at the start of the buffer as a special case.

70f74ba

Get rid of unneeded terminal api methods.

bd58211

Update tests to account for removed APIs.

fbe9f5d

Update window background when the color table changes.

2a9a210

j4james force-pushed the merge-dispatch-classes branch from 50c87e3 to 2a9a210 Compare May 2, 2022 23:51

j4james added 2 commits May 3, 2022 02:01

Make the audit build happy.

1fd0dff

Happy now?

a2b6942

lhecker reviewed May 3, 2022

View reviewed changes

j4james added 2 commits May 3, 2022 17:54

Some PR feedback.

984236a

More PR feedback.

1320a22

DHowett reviewed May 3, 2022

View reviewed changes

src/terminal/adapter/ut_adapter/adapterTest.cpp Show resolved Hide resolved

zadjii-msft mentioned this pull request May 3, 2022

Feature Request: sixel graphics support #448

Closed

Make sure line rendition ops aren't unintentionally passed through co…

5eed6a5

…npty.

j4james marked this pull request as ready for review May 4, 2022 00:27

DHowett approved these changes May 4, 2022

View reviewed changes

lhecker approved these changes May 4, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 4, 2022

ghost merged commit f4e0d9f into microsoft:main May 4, 2022

This was referenced May 4, 2022

Move InputStateMachineEngine and InteractDispatch into Host #13036

Open

Resizing while alt buffer is active, then disabling it, blows away renderer #13038

Closed

j4james mentioned this pull request May 5, 2022

Stop the viewport being moved when in the alt buffer #13039

Merged

6 tasks

j4james deleted the merge-dispatch-classes branch May 14, 2022 12:48

ghost mentioned this pull request May 24, 2022

De-duplicate code for TerminalDispatch and AdaptDispatch #3849

Closed

1 task

j4james mentioned this pull request Jun 10, 2022

Terminal can omit backgrounds, links and underlines on empty cells #13229

Closed

DHowett mentioned this pull request Jun 30, 2022

Close the gap between Terminal and Conhost ITerminalApi #13408

Open

This was referenced Aug 19, 2022

Suggestions pop up of the Japanese MS IME got pushed down after multiple commands #13769

Closed

Use the viewport-relative cursor pos for CCore.CursorPosition #13785

Merged

DHowett mentioned this pull request Aug 19, 2022

[1.14] Use the viewport-rel. cursor pos for CursorPosition #13786

Merged

j4james mentioned this pull request Sep 11, 2022

Reset disables the ability to pass through C1 control characters #13968

Closed

DHowett mentioned this pull request Jul 23, 2024

A minor ConPTY refactoring: Goodbye VtEngine Edition #17510

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge the TerminalDispatch and AdaptDispatch classes#13024

Merge the TerminalDispatch and AdaptDispatch classes#13024
17 commits merged intomicrosoft:mainfrom
j4james:merge-dispatch-classes

j4james commented May 2, 2022 •

edited

Loading

Uh oh!

DHowett commented May 2, 2022

Uh oh!

j4james commented May 3, 2022

Uh oh!

lhecker commented May 3, 2022

Uh oh!

lhecker left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Uh oh!

Uh oh!

j4james commented May 3, 2022

Uh oh!

DHowett left a comment

Uh oh!

zadjii-msft commented May 4, 2022

Uh oh!

ghost commented May 4, 2022

Uh oh!

ghost commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

j4james commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

DHowett commented May 2, 2022

Uh oh!

j4james commented May 3, 2022

Uh oh!

lhecker commented May 3, 2022

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j4james commented May 3, 2022

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented May 4, 2022

Uh oh!

ghost commented May 4, 2022

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.

Uh oh!

ghost commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

j4james commented May 2, 2022 •

edited

Loading

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.