microsoft / microsoft/terminal

Close the gap between Terminal and Conhost ITerminalApi

Open
#13,408 7 comments 0 reactions 0 assignees View on GitHub
Area-VT Issue-Scenario Priority-2 Product-Terminal
Dominant language
C++
Stars
105k
Forks
9.6k
Avg merge
3d 17h
Merged PRs (30d)
29

Description

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.

Contributor guide

Open the contributing guide

Research direction

Start with Terminal's ITerminalApi TODO methods and compare Terminal's _WriteBuffer and _AdjustCursorPosition with conhost's WriteCharsLegacy and AdjustCursorPosition. Review the notes about AdaptDispatch and Build-SupportedSequenceIndex; done requires a defined, agreed scope for unifying the implementations and tracking sequences supported by only one host.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
cli, operating-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.