starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Close the gap between Terminal and Conhost ITerminalApi #17836

New Issue
Open
opened 2026-01-31 05:55:58 +00:00 by claunia · 7 comments
claunia commented 2026-01-31 05:55:58 +00:00
Owner
Copy Link

Originally created by @DHowett on GitHub (Jun 30, 2022).

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. 😄

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.

Originally created by @DHowett on GitHub (Jun 30, 2022). 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.
claunia added the Area-VTProduct-TerminalPriority-2Issue-Scenario labels 2026-01-31 05:55:58 +00:00
claunia commented 2026-01-31 05:55:59 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jun 30, 2022):

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

@DHowett commented on GitHub (Jun 30, 2022): There are also things that Terminal supports which conhost does not. I am not sure whether it is right to track them here.
claunia commented 2026-01-31 05:55:59 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jun 30, 2022):

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).

@DHowett commented on GitHub (Jun 30, 2022): 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).
claunia commented 2026-01-31 05:55:59 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jun 30, 2022):

@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 commented on GitHub (Jun 30, 2022): > @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.
claunia commented 2026-01-31 05:56:00 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jan 3, 2023):

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 commented on GitHub (Jan 3, 2023): 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.
claunia commented 2026-01-31 05:56:00 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jan 5, 2023):

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.

@j4james commented on GitHub (Jan 5, 2023): 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.
claunia commented 2026-01-31 05:56:00 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jan 5, 2023):

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.

@zadjii-msft commented on GitHub (Jan 5, 2023): 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.
claunia commented 2026-01-31 05:56:01 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Jan 18, 2023):

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.

@lhecker commented on GitHub (Jan 18, 2023): > 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-VT Issue-Scenario Priority-2 Product-Terminal
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#17836
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?