Repository navigation
Close the gap between Terminal and Conhost ITerminalApi #13408
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.
James Holderness (@j4james) are there any that I've missed? I was pretty hamfisted with it. 😄
Notes from James Holderness (@j4james):
That looks right from the point of view of
ITerminalApi. But I think the main things outstanding are that the_WriteBufferand_AdjustCursorPositionmethods inTerminalneed to be unified with theWriteCharsLegacyandAdjustCursorPositionfunctions from conhost, since theTerminalversions are missing a bunch of functionality.And once those methods are merged into
AdaptDispatch, we might also be able to get rid of someITerminalApimethods likeSetScrollingRegion,GetLineFeedMode, andLineFeed.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:
EnableXtermBracketedPasteModeCopyToClipboardSetTaskbarProgressSetWorkingDirectoryAddMarkAlthough in the case of
SetWorkingDirectory, I'm not sure there's anything useful conhost can do with that information. AndAddMarkshould really be moved out ofITerminalApiand implemented directly on theTextBuffersomehow. As currently implemented, it's kind of blocking theAdjustCursorPositionunification.Also,
Build-SupportedSequenceIndexstill expects to findterminalDispatch. Right now, we don't have a way to tell it that Terminal doesn't support a particular control sequenceYeah, I was thinking we should maybe come up with a new way of tracking that info - maybe with a well-defined pattern in the
AdaptDispatchdoc 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.
Activity
DHowett commentedon 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 commentedon Jun 30, 2022
Also,
Build-SupportedSequenceIndexstill expects to findterminalDispatch. 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 commentedon Jun 30, 2022
That looks right from the point of view of
ITerminalApi. But I think the main things outstanding are that the_WriteBufferand_AdjustCursorPositionmethods inTerminalneed to be unified with theWriteCharsLegacyandAdjustCursorPositionfunctions from conhost, since theTerminalversions are missing a bunch of functionality.And once those methods are merged into
AdaptDispatch, we might also be able to get rid of someITerminalApimethods likeSetScrollingRegion,GetLineFeedMode, andLineFeed.I didn't think anyone really cared about the conhost side, but these are the ones I'm aware of:
EnableXtermBracketedPasteModeCopyToClipboardSetTaskbarProgressSetWorkingDirectoryAddMarkAlthough in the case of
SetWorkingDirectory, I'm not sure there's anything useful conhost can do with that information. AndAddMarkshould really be moved out ofITerminalApiand implemented directly on theTextBuffersomehow. As currently implemented, it's kind of blocking theAdjustCursorPositionunification.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
AdaptDispatchdoc 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 commentedon Jan 3, 2023
I've been experimenting with the
WriteCharsLegacy/_WriteBufferpart 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 ofITerminalApi::LineFeedto handle the line wrapping (the modification is because aLineFeedcall usually clears the WrapForced flag, while in this case it needs to be set).This way we don't need to touch
WriteCharsLegacy, andAdjustCursorPositionis only used indirectly via theLineFeedAPI (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 forWriteCharsLegacy2ElectricBoogaloo(#780), which I see is now assigned to Leonard Hecker (@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
AdaptDispatchso I can progress with VT features that depends on that.j4james commentedon 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 theOutputCellIterator), and then scrolls the target area right by that amount before callingTextBuffer::WriteLine.I know it's not ideal, and I'm assuming the ultimate plan is to handle the inserting in the
TextBufferclass itself, but it's only a couple of lines, and it should be easy to replace with something like a flag on theWriteLinemethod once that functionality is available. In the meantime, though, we'll at least have something working.zadjii-msft commentedon 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 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 commentedon Jan 18, 2023
James Holderness (@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
WriteCharsLegacyrewrite, I've got a very long list of other areas that I was planning to address along with it here: #8000 (comment). If you intend to continue working onWriteCharsLegacyor related code, I don't mind working on these other bits first.Merge the PrintString functionality into AdaptDispatch (#14640)
Merge the LineFeed functionality into AdaptDispatch (#14874)
Add support for bracketed paste mode in ConHost (#15155)