Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close the gap between Terminal and Conhost ITerminalApi #13408

Open
Tracked by #6999 ...
DHowett opened this issue Jun 30, 2022 · 8 comments
Open
Tracked by #6999 ...

Close the gap between Terminal and Conhost ITerminalApi #13408

DHowett opened this issue Jun 30, 2022 · 8 comments
Labels
Area-VT Virtual Terminal sequence support Issue-Scenario Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Milestone

Comments

@DHowett
Copy link
Member

DHowett commented 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.

@DHowett DHowett added Product-Terminal The new Windows Terminal. Priority-2 A description (P2) Issue-Scenario labels Jun 30, 2022
@DHowett DHowett added this to the Backlog milestone Jun 30, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 30, 2022
@DHowett
Copy link
Member Author

DHowett commented 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
Copy link
Member Author

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

@j4james
Copy link
Collaborator

j4james commented 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.

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 5, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 5, 2022
@j4james
Copy link
Collaborator

j4james commented 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
Copy link
Collaborator

j4james commented 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.

@zadjii-msft
Copy link
Member

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
Copy link
Member

lhecker commented 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: #8000 (comment). If you intend to continue working on WriteCharsLegacy or related code, I don't mind working on these other bits first.

ghost pushed a commit that referenced this issue Jan 18, 2023
The main purpose of this PR was to merge the `ITerminalApi::PrintString`
implementations into a shared method in `AdaptDispatch`, and avoid the
VT code path depending on `WriteCharsLegacy`. But this refactoring has
also fixed some bugs that existed in the original implementations. 

This helps to close the gap between the Conhost and Terminal (#13408).

I started by taking the `WriteCharsLegacy` implementation, and stripping
out everything that didn't apply to the VT code path. What was left was
a fairly simple loop with the following steps:

1. Check if _delayed wrap_ is set, and if so, move to the next line.
2. Write out as much of the string as will fit on the current line.
3. If we reach the end of the line, set the _delayed wrap_ flag again.
4. Repeat the loop until the entire string has been output.

But step 2 was a little more complicated than necessary because of its
legacy history. It was copying the string into a temporary buffer,
manually estimated how much of it would fit, and then passing on that
partial buffer to the `TextBuffer::Write` method.

In the new implementation, we just pass the entire string directly to
`TextBuffer::WriteLine`, and that handles the clipping itself. The
returned `OutputCellIterator` tells us how much of the string is left.
This approach fixes some issues with wide characters, and should also
cope better with future buffer enhancements.

Another improvement from the new implementation is that the Terminal now
handles delayed EOL wrap correctly. However, the downside of this is
that it introduced a cursor-dropping bug that previously only affected
conhost. I hadn't originally intended to fix that, but it became more of
an issue now.

The root cause was the fact that we called `cursor.StartDeferDrawing()`
before outputting the text, and this was something I had adopted in the
new implementation as well. But I've now removed that, and instead just
call `cursor.SetIsOn(false)`. This seems to avoid the cursor droppings,
and hopefully still has similar performance benefits.

The other thing worth mentioning is that I've eliminated some special
casing handling for the `ENABLE_VIRTUAL_TERMINAL_PROCESSING` mode and
the `WC_DELAY_EOL_WRAP` flag in the `WriteCharsLegacy` function. They
were only used for VT output, so aren't needed here anymore.

## Validation Steps Performed

I've just been testing manually, writing out sample text both in ASCII
and with wide Unicode chars. I've made sure it wraps correctly when
exceeding the available space, but doesn't wrap when stopped at the last
column, and with `DECAWM` disabled, it doesn't wrap at all.

I've also confirmed that the test case from issue #12739 is now working
correctly, and the cursor no longer disappears in Windows Terminal when
writing to the last column (i.e. the delayed EOL wrap is working).

Closes #780
Closes #6162
Closes #6555
Closes #12440
Closes #12739
DHowett pushed a commit that referenced this issue Mar 30, 2023
The main purpose of this PR was to merge the `ITerminalApi::LineFeed`
implementations into a shared method in `AdaptDispatch`, and avoid the
VT code path depending on the `AdjustCursorPosition` function (which
could then be massively simplified). This refactoring also fixes some
bugs that existed in the original `LineFeed` implementations.

## References and Relevant Issues

This helps to close the gap between the Conhost and Terminal (#13408).
This improves some of the scrollbar mark bugs mentioned in #11000.

## Detailed Description of the Pull Request / Additional comments

I had initially hoped the line feed functionality could be implemented
entirely within `AdaptDispatch`, but there is still some Conhost and
Terminal-specific behavior that needs to be triggered when we reach the
bottom of the buffer, and the row coordinates are cycled.

In Conhost we need to trigger an accessibility scroll event, and in
Windows Terminal we need to update selection and marker offsets, reset
pattern intervals, and preserve the user's scroll offset. This is now
handled by a new `NotifyBufferRotation` method in `ITerminalApi`.

But this made me realise that the `_EraseAll` method should have been
doing the same thing when it reached the bottom of the buffer. So I've
added a call to the new `NotifyBufferRotation` API from there as well.

And in the case of Windows Terminal, the scroll offset preservation was
something that was also needed for a regular viewport pan. So I've put
that in a separate `_PreserveUserScrollOffset` method which is called
from the `SetViewportPosition` handler as well.

## Validation Steps Performed

Because of the API changes, there were a number of unit tests that
needed to be updated:

- Some of the `ScreenBufferTests` were accessing margin state in the
`SCREEN_INFORMATION` class which doesn't exist anymore, so I had to add
a little helper function which now manually detects the active margins.

- Some of the `AdapterTest` tests were dependent on APIs that no longer
exist, so they needed to be rewritten so they now check the resulting
state rather than expecting a mock API call.

- The `ScrollWithMargins` test in `ConptyRoundtripTests` was testing
functionality that didn't previously work correctly (issue #3673). Now
that it's been fixed, that test needed to be updated accordingly.

Other than getting the unit tests working, I've manually verified that
issue #3673 is now fixed. And I've also checked that the scroll markers,
selections, and user scroll offset are all being updated correctly, both
with a regular viewport pan, as well as when overrunning the buffer.

Closes #3673
DHowett pushed a commit that referenced this issue Apr 26, 2023
This adds support for XTerm's "bracketed paste" mode in ConHost. When
enabled, any pasted text is bracketed with a pair of escape sequences,
which lets the receiving application know that the content was pasted
rather than typed.

## References and Relevant Issues

Bracketed paste mode was added to Windows Terminal in PR #9034.
Adding it to ConHost ticks one more item off the list in #13408. 

## Detailed Description of the Pull Request / Additional comments

This only applies when VT input mode is enabled, since that is the way
Windows Terminal currently works.

When it comes to filtering, though, the only change I've made is to
filter out the escape character, and only when bracketed mode is
enabled. That's necessary to prevent any attempts to bypass the
bracketing, but I didn't want to mess with the expected behavior for
legacy apps if bracketed mode is disabled.

## Validation Steps Performed

Manually tested in bash with `bind 'set enable-bracketed-paste on'` and
confirmed that pasted content is now buffered, instead of being executed
immediately.

Also tested in VIM, and confirmed that you can now paste preformatted
code without the autoindent breaking the formatting.

Closes #395
@mominshaikhdevs
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Scenario Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

5 participants