Close the gap between Terminal and Conhost ITerminalApi

[DHowett]

There's a number of TODOs in Terminal's implementation of ITerminalApi, herein captured:

58-void Terminal::SetAutoWrapMode(const bool /*wrapAtEOL*/)
59-{
60:    // TODO: This will be needed to support DECAWM.

63-void Terminal::SetScrollingRegion(const til::inclusive_rect& /*scrollMargins*/)
64-{
65:    // TODO: This will be needed to fully support DECSTBM.

73-bool Terminal::GetLineFeedMode() const
74-{
75:    // TODO: This will be needed to support LNM.

109-bool Terminal::ResizeWindow(const til::CoordType /*width*/, const til::CoordType /*height*/)
110-{
111:    // TODO: This will be needed to support various resizing sequences. See also GH#1860.

115-void Terminal::SetConsoleOutputCP(const unsigned int /*codepage*/)
116-{
117:    // TODO: This will be needed to support 8-bit charsets and DOCS sequences.

120-unsigned int Terminal::GetConsoleOutputCP() const
121-{
122:    // TODO: See SetConsoleOutputCP above.

These were introduced in the merger of TerminalDispatch and AdaptDispatch, which unified conhost's and Terminal's output state machine engine dispatchers in and around #13024.

~~@j4james are there any that I've missed? I was pretty hamfisted with it. :smile:~~

Notes from @j4james:

That looks right from the point of view of ITerminalApi. But I think the main things outstanding are that the _WriteBuffer and _AdjustCursorPosition methods in Terminal need to be unified with the WriteCharsLegacy and AdjustCursorPosition functions from conhost, since the Terminal versions are missing a bunch of functionality.

And once those methods are merged into AdaptDispatch, we might also be able to get rid of some ITerminalApi methods like SetScrollingRegion, GetLineFeedMode, and LineFeed.

There are also things that Terminal supports which conhost does not. I am not sure whether it is right to track them here.

I didn't think anyone really cared about the conhost side, but these are the ones I'm aware of:

• EnableXtermBracketedPasteMode
• CopyToClipboard
• SetTaskbarProgress
• SetWorkingDirectory
• AddMark

Although in the case of SetWorkingDirectory, I'm not sure there's anything useful conhost can do with that information. And AddMark should really be moved out of ITerminalApi and implemented directly on the TextBuffer somehow. As currently implemented, it's kind of blocking the AdjustCursorPosition unification.

Also, Build-SupportedSequenceIndex still expects to find terminalDispatch. Right now, we don't have a way to tell it that Terminal doesn't support a particular control sequence

Yeah, I was thinking we should maybe come up with a new way of tracking that info - maybe with a well-defined pattern in the AdaptDispatch doc comments. That way we would have more room to add annotations like "ConHost-only" or "Terminal-only" for operations that aren't yet supported in both.

[DHowett]

There are also things that Terminal supports which conhost does not. I am not sure whether it is right to track them here.

[DHowett]

Also, Build-SupportedSequenceIndex still expects to find terminalDispatch. Right now, we don't have a way to tell it that Terminal doesn't support a particular control sequence (somewhat by design, since the dispatcher is of course identical).

[j4james]

@j4james are there any that I've missed? I was pretty hamfisted with it. 😄

That looks right from the point of view of ITerminalApi. But I think the main things outstanding are that the _WriteBuffer and _AdjustCursorPosition methods in Terminal need to be unified with the WriteCharsLegacy and AdjustCursorPosition functions from conhost, since the Terminal versions are missing a bunch of functionality.

And once those methods are merged into AdaptDispatch, we might also be able to get rid of some ITerminalApi methods like SetScrollingRegion, GetLineFeedMode, and LineFeed.

There are also things that Terminal supports which conhost does not. I am not sure whether it is right to track them here.

I didn't think anyone really cared about the conhost side, but these are the ones I'm aware of:

• EnableXtermBracketedPasteMode
• CopyToClipboard
• SetTaskbarProgress
• SetWorkingDirectory
• AddMark

Although in the case of SetWorkingDirectory, I'm not sure there's anything useful conhost can do with that information. And AddMark should really be moved out of ITerminalApi and implemented directly on the TextBuffer somehow. As currently implemented, it's kind of blocking the AdjustCursorPosition unification.

Also, Build-SupportedSequenceIndex still expects to find terminalDispatch. Right now, we don't have a way to tell it that Terminal doesn't support a particular control sequence

Yeah, I was thinking we should maybe come up with a new way of tracking that info - maybe with a well-defined pattern in the AdaptDispatch doc comments. That way we would have more room to add annotations like "ConHost-only" or "Terminal-only" for operations that aren't yet supported in both.

[j4james]

I've been experimenting with the WriteCharsLegacy/_WriteBuffer part of this, and I think I have a solution that could be turned into a PR if you're interested.

What it amounts to is a loop over the string that is being output, writing a line at a time with TextBuffer::WriteLine, and calling a slightly modified version of ITerminalApi::LineFeed to handle the line wrapping (the modification is because a LineFeed call usually clears the WrapForced flag, while in this case it needs to be set).

This way we don't need to touch WriteCharsLegacy, and AdjustCursorPosition is only used indirectly via the LineFeed API (hopefully that dependency will be removed in a later PR).

But with this functionality merged into AdaptDispatch, I'm assuming there's no longer a need for WriteCharsLegacy2ElectricBoogaloo (#780), which I see is now assigned to @lhecker . So I before I submit anything, I just want to make sure I'm not doing something that's going to mess with your existing plans.

I should also stress out that I haven't tried to address any performance issues, or fix bugs like the cursor droppings (#12739). I'm just trying to get the existing functionality moved into AdaptDispatch so I can progress with VT features that depends on that.

[j4james]

Btw, I've also got a trivial implementation of IRM (#1947) built on top of this. When that mode is enabled, it first measures how many cells the string is expected to take (using the OutputCellIterator), and then scrolls the target area right by that amount before calling TextBuffer::WriteLine.

I know it's not ideal, and I'm assuming the ultimate plan is to handle the inserting in the TextBuffer class itself, but it's only a couple of lines, and it should be easy to replace with something like a flag on the WriteLine method once that functionality is available. In the meantime, though, we'll at least have something working.

[zadjii-msft]

Leonard's gonna be out for a little while now, but IIRC, there's nothing too concrete in the works quite yet. Pretty sure we just bulk-assigned all the buffer writing stuff to him 😝 Getting rid of the need for that would probably just make his life easier. I'd say go for it.

I know it's not ideal,

I honestly could care less if it's ideal. I don't think IRM is gonna be used in any truly hot paths for raw throughput, so I'd rather have a less-than-ideal solution than nothing at all.

[lhecker]

So I before I submit anything, I just want to make sure I'm not doing something that's going to mess with your existing plans.

@j4james Don't worry about me. Your work is absolutely fantastic. :) I've been sick since the start of the year and my recovery has been slower than I had hoped, so it'll take a little bit for me to catch up anyways. Additionally I'm planning to prioritize fixing the remaining AtlasEngine issues a bit, so that we can finally release it enabled by default and delete DxEngine sooner rather than later (to reduce the maintenance burden). Also, related to the WriteCharsLegacy rewrite, I've got a very long list of other areas that I was planning to address along with it here: https://github.com/microsoft/terminal/issues/8000#issuecomment-1325410702. If you intend to continue working on WriteCharsLegacy or related code, I don't mind working on these other bits first.

[mominshaikhdevs]

I believe this issue #1765 (ConPTY sends two WINDOW_BUFFER_SIZE_EVENT messages when the window is restored from maximize) belongs here too.