microsoft / microsoft/terminal
Close the gap between Terminal and Conhost ITerminalApi
- 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
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