Further refactor and simplify the ConGetSet API #12703

j4james · 2022-03-15T22:32:10Z

This is an attempt to simplify the ConGetSet interface down to the
smallest set of methods necessary to support the AdaptDispatch class.
The idea is that it should then be easier to implement that interface in
Windows Terminal, so we can replace the TerminalDispatch class with
AdaptDispatch.

This is a continuation of the refactoring started in #12247, and a
significant step towards #3849.

Detailed Description of the Pull Request / Additional comments

The general idea was to give the AdaptDispatch class direct access to
the high-level structures on which it needs to operate. Some of these
structures are now passed in when the class is constructed (the
Renderer, RenderSettings, and TerminalInput), and some are exposed
as new methods in ConGetSet (GetStateMachine, GetTextBuffer, and
GetViewport).

Many of the existing ConhostInternalGetSet methods could easily then
be reimplemented in AdaptDispatch, since they were often simply
forwarding to methods in one of the above structures. Some were a little
more complicated, though, and require further explanation.

GetConsoleScreenBufferInfoEx: What we were typically using this for
was to obtain the viewport, although what we really wanted was the
virtual viewport, which is now accessible via the GetViewport
method. This was also used to obtain the cursor position and buffer
width, which we can now get via the GetTextBuffer method.
SetConsoleScreenBufferInfoEx: This was only really used for the
AdaptDispatch::SetColumns implementation (for DECCOLM mode), and
that could be replaced with ResizeWindow. This is a slight change in
behaviour (it sizes the window rather than the buffer), but neither is
technically correct for DECCOLM, so I think it's good enough for
now, and at least it's consistent with the other VT sizing operations.
SetCursorPosition: This could mostly be replaced with direct
manipulation of the Cursor object (accessible via the text buffer),
although again this is a slight change in behavior. The original code
would also have made a call to ConsoleImeResizeCompStrView (which I
don't think is applicable to VT movement), and would potentially have
moved the viewport (not essential for now, but could later be
supported by DECHCCM). It also called VtIo::SetCursorPosition to
handle cursor inheritance, but that should only apply to
InteractDispatch, so I've moved that to the
InteractDispatch::MoveCursor method.
ScrollRegion: This has been replaced by two simple helper methods in
AdaptDispatch which better meet the VT requirements -
_ScrollRectVertically and _ScrollRectHorizontally. Unlike the
original ScrollRegion implementation, these don't generate
EVENT_CONSOLE_UPDATE_SCROLL events (see ScrollRegion function generates inappropriate EVENT_CONSOLE_UPDATE_SCROLL #12656 for more details).
FillRegion: This has been replaced by the _FillRect helper method
in AdaptDispatch. It differs from the original FillRegion in that
it takes a rect rather than a start position and length, which gives
us more flexibility for future operations.
ReverseLineFeed: This has been replaced with a somewhat refactored
reimplementation in AdaptDispatch, mostly using the
_ScrollRectVertically helper described above.
EraseAll: This was previously handled by
SCREEN_INFORMATION::VtEraseAll, but has now been entirely
reimplemented in the AdaptDispatch::_EraseAll method.
DeleteLines/InsertLines/_modifyLines: These have been replaced
by the _InsertDeleteLineHelper method in AdaptDispatch, which
mostly relies on the _ScrollRectVertically helper described above.

Finally there were a few methods that weren't actually needed in the
ConGetSet interface:

MoveToBottom: This was really just a hack to get the virtual
viewport from GetConsoleScreenBufferInfoEx. We may still want
something like in the future (e.g. to support DECVCCM or Option to cause window to automatically scroll to bottom when new output is received #8879), but
I don't think it's essential for now.
SuppressResizeRepaint: This was only needed in InteractDispatch
and PtySignalInputThread, and they could easily access the VtIo
object to implement it themselves.
ClearBuffer: This was only used in PtySignalInputThread, and that
could easily access the buffer directly via the global console
information.
WriteControlInput: This was only used in InteractDispatch, and
that could easily be replaced with a direct call to
HandleGenericKeyEvent.

As part of these changes, I've also refactored some of the existing
AdaptDispatch code:

_InsertDeleteHelper (renamed _InsertDeleteCharacterHelper) is now
just a straightforward call to the new _ScrollRectHorizontally
helper.
EraseInDisplay and EraseInLine have been implemented as a series
of _FillRect calls, so _EraseSingleLineHelper is no longer
required.
_EraseScrollback is a essentially a special form of scrolling
operation, which mostly depends on the TextBuffer::ScrollRows
method, and with the filling now provided by the new _FillRect
helper.
There are quite a few operations now in AdaptDispatch that are
affected by the scrolling margins, so I've pulled out the common
margin setup into a new _GetVerticalMargins helper method. This also
fixes some edge cases where margins could end up out of range.

Validation Steps Performed

There were a number of unit tests that needed to be updated to work
around functions that have now been removed, but these substitutions
were fairly straightforward for the most part.

The adapter tests were a different story, though. In that case we were
explicitly testing how operations were passed through to the ConGetSet
interface, but with more than half those methods now gone, a significant
rewrite was required.

I've tried to retain the crux of the original tests, but we now have to
validate the state changes on the underlying data structures, where
before that state would have been tracked in the TestGetSet mock. And
in some cases we were testing what happened when a method failed, but
since that scenario is no longer possible, I've simply removed those
tests.

I've also tried to manually test all the affected operations to confirm
that they're still working as expected, both in vttest as well as my own
test scripts.

Closes #12662

j4james · 2022-03-22T01:17:56Z

There's more I'd still like to do to simplify ConGetSet even further, but I think this is good enough to move on to the TerminalDispatch replacement phase, which is more of a priority.

I know this is a big change, and there's a definite chance it might break something, but I do think it's worth the risk. Because once we've got a more complete VT implementation on the Terminal side, the pass-through mode starts to look like a realistic prospect.

lhecker · 2022-03-23T23:54:25Z

FYI This week we're all a bit preoccupied with projects outside of regular Terminal work (something called "Fix Hack Learn"). Personally, I'll be on vacation next week, so I'll unfortunately only get a chance to take a look at this in about two weeks (unless it's already merged by then).

zadjii-msft · 2022-03-29T16:17:00Z

Taking a closer look at this today. Some of the work that's happening in the #12515 and #12526 intersection will likely run across the congetset boundary, so IMO we should merge this in first, and let me & @miniksa deal with the fallout ourselves.

zadjii-msft

Wow, this is quite the read. I've got minor questions scattered about. I don't think I have any blocking concerns. Everything looks like it found a new home.

Really looking forward to having one less layer to deal with after all these years.

zadjii-msft · 2022-03-29T18:08:28Z

src/terminal/adapter/adaptDispatch.cpp

+    else if (cursorPosition.Y > viewport.top)
+    {
+        // Otherwise we move the cursor up, but not past the top of the viewport.
+        const COORD newCursorPosition{ cursorPosition.X, cursorPosition.Y - 1 };


Does this actually clamp to the viewport? ClampPositionWithinLine doesn't seem to clamp the y position, only the x

See the condition above (cursorPosition.Y > viewport.top). If Y is greater than the viewport top, then Y-1 will still be greater than or equal to the top, which is still in range.

zadjii-msft · 2022-03-29T18:09:39Z

src/host/outputStream.cpp

-    if (oldCursorPosition.Y > viewport.Top)
-    {
-        // Cursor is below the top line of the viewport
-        THROW_IF_NTSTATUS_FAILED(AdjustCursorPosition(screenInfo, newCursorPosition, TRUE, nullptr));


I suppose before AdjustCursorPosition would make sure to move the viewport to make the cursor visible, but it was never moving outside of the viewport, so that's not an issue.

NOTE!: The call to InitializeCursorRowAttributes though - does that still happen?

In this particular situation it wouldn't apply, because ReverseLineFeed shouldn't be panning the viewport when it moves the cursor. That said, When it reaches the top, it'll trigger a scroll of the buffer content, and that does do something similar to InitializeCursorRowAttributes for the revealed line. See the calls to _FillRect and ResetLineRenditionRange in the _ScrollRectVertically method.

zadjii-msft · 2022-03-29T18:11:32Z

src/terminal/adapter/adaptDispatch.cpp

+        cursor.SetPosition(textBuffer.ClampPositionWithinLine(newCursorPosition));
+        cursor.SetDelay(false);
+        cursor.SetIsOn(true);
+        cursor.SetHasMoved(true);


Were these guys getting done before? (sometimes hard to parse all the side effects of AdjustCursorPosition)

zadjii-msft · 2022-03-29T18:55:33Z

src/terminal/adapter/adaptDispatch.cpp

+        cursor.SetXPosition(0);
+        cursor.SetDelay(false);
+        cursor.SetIsOn(true);
+        cursor.SetHasMoved(true);


Maybe we need a helper for set cursor position and {delay, on, moved}={false, true, true}, since this is the second time I'm seeing it now.

Yeah. I was considering that, but I thought maybe we could push that off for a follow up issue because I think this cursor handling needs further investigation.

If these calls are needed all the time, then why aren't they set automatically in the SetPosition method? If they aren't needed all the time, then we may need to be more selective about where they are used, and dumping these calls in a helper is just hiding the problem.

And note that Windows Terminal doesn't use these flags at all as far as I can tell, so I'm hoping it might be possible to do away with them altogether.

FYI, I eventually decided you were right about this and added a helper for setting these flags. It does make things a bit tidier and gave me the opportunity to add a bit of documentation on what we are doing here (at least as I understand it). See commit 446002e.

zadjii-msft · 2022-03-29T20:09:59Z

src/terminal/adapter/adaptDispatch.cpp

+            const auto data = OutputCell(*textBuffer.GetCellDataAt(sourcePos));
+            textBuffer.Write(OutputCellIterator({ &data, 1 }), targetPos);
+            source.WalkInBounds(sourcePos, walkDirection);
+        } while (target.WalkInBounds(targetPos, walkDirection));


Coreect me if I'm wrong here - if we had a buffer like:

1234567890

And the cursor was on the 1, and we inserted 5 characters, we'd call this with (textBuffer, { 0, 0, 80, 1 }, 5)

Actually nevermind me. That works perfectly fine. I thought we'd start reading from the section we were writing from, so it'd look like

12341234

or something, but nah this works fine.

I think I just stole that code from the existing _CopyRectangle function:

terminal/src/host/output.cpp

Lines 107 to 111 in ce6ce50

const auto data = OutputCell(*screenInfo.GetCellDataAt(sourcePos));

screenInfo.Write(OutputCellIterator({ &data, 1 }), targetPos);

source.WalkInBounds(sourcePos, walkDirection);

} while (target.WalkInBounds(targetPos, walkDirection));

zadjii-msft · 2022-03-29T20:24:24Z

src/terminal/adapter/adaptDispatch.cpp

+    cursor.SetXPosition(column);
+    cursor.SetDelay(false);
+    cursor.SetIsOn(true);
+    cursor.SetHasMoved(true);


Ah okay I found it. These come from: SCREEN_INFORMATION::SetCursorPosition

terminal/src/host/screenInfo.cpp

Lines 1728 to 1740 in 0651d92

if (gci.Flags & CONSOLE_HAS_FOCUS)

{

if (TurnOn)

{

cursor.SetDelay(false);

cursor.SetIsOn(true);

}

else

{

cursor.SetDelay(true);

}

cursor.SetHasMoved(true);

}

IsOn is always passed as true in ApiRoutines::SetConsoleCursorPositionImpl, so this right here is assuming that the console has focus.... that's probably okay. I don't entirely know the origin of that HAS_FOCUS check.

This may be served well by having a helper - I think I'm gonna have to introduce focus tracking even for conpty fairly soon.

My impression was the the HAS_FOCUS check was just an optimization, and it shouldn't harm to set these flags anyway. The TimerRoutine that processes this stuff has it's own HAS_FOCUS check that prevents it triggering a notification if the window isn't focused.

terminal/src/host/CursorBlinker.cpp

Lines 86 to 89 in ce6ce50

if (!WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))

{

goto DoScroll;

}

If anything, I think the existing HAS_FOCUS check might cause it lose some move events, and could be a bug. But again I thought this stuff should probably be investigated further in a follow up issue. I didn't want to get too bogged down trying to resolve side issues like this if I could avoid it. As long as we aren't making thing worse in the meantime.

zadjii-msft · 2022-03-29T21:01:38Z

Another project maintenance note: We've been largely focused on fixes that we can service back to 1.12, for syncing fixes with the OS build of the Terminal. This seems like it would firmly fit outside the realm of servicable fixes, so things that build off this may have a much harder time getting serviced to 1.12.

Looking through the 1.14 list, there's like, two categories where this might matter:

Alt buffer stuff - I don't think we're planning on bringing that back to 1.12 anymore, so ezpz
the z-order / foreground / showhide stuff. This is... complicated
- Plumbing FocusIn/FocusOut from the Terminal to conpty via InteractDispatch: before this PR, it'll require a new congetset method. After this PR, the body of that congetset method could certainly just get moved into InteractDispatch directly. So not the worst to have separate versions of that.
- Same with the Show/Hide stuff, for ConhostInternalGetSet::ShowWindow.

Okay, not that complicated. Definitely doable, annoying, but doable if we want the FG stuff in 1.12. @DHowett as fyi

j4james · 2022-03-29T23:05:57Z

the z-order / foreground / showhide stuff. This is... complicated

I don't mind if you want to do those first. I only looked at them briefly, but I didn't think they'd be a huge problem to merge.

zadjii-msft · 2022-04-13T16:06:01Z

I'm cool merging this. I'm a big kid, I can deal with the conflicts in my own branches. Happy to get this one in and get you unblocked ☺️

That being said, literally every time I've tried pushing a merge fix to a branch from a fork, I'm botched it horribly.

DHowett · 2022-04-13T17:29:47Z

@msftbot make sure @DHowett signs off

@j4james I am happy to deal with the conflict if you don't have time to do so :)

ghost · 2022-04-13T17:29:52Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett and @j4james

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

j4james · 2022-04-13T18:14:18Z

@DHowett I'm still at work at the moment, but I should be able to look at it later tonight. I was hoping to try and address the issues that Leonard raised at the same time, although they probably aren't essential if you want to get this over and done with.

DHowett

I'm at 26/28, and I want to go get lunch. I'll swing back by for the other 2. I'm not surprised that I only have one comment so far -- this is top-notch work, as always. :)

DHowett · 2022-04-13T19:05:57Z

src/terminal/adapter/ut_adapter/Adapter.UnitTests.vcxproj

@@ -28,6 +28,9 @@
    <ProjectReference Include="..\..\..\interactivity\base\lib\InteractivityBase.vcxproj">
      <Project>{06ec74cb-9a12-429c-b551-8562ec964846}</Project>
    </ProjectReference>
+    <ProjectReference Include="..\..\..\renderer\base\lib\base.vcxproj">


Taking a mental note to adjust the link line in the sources file to include ConRenderBase (thanks for calling this one to my attention!)

I've done the merge and the other updates. Would you like me to do this too or should I leave it for you to fix? I'm not exactly sure what is required, but I thought maybe I just had to add back this line:

$(WINCORE_OBJ_PATH)\console\open\src\renderer\base\lib\$(O)\ConRenderBase.lib \

Or is there more to it than that?

There's a nonzero chance there's more to it than that (Base might depend on Interactivity, which would bring back in the whole kit&kaboodle). I'm happy to handle this one, but thank you!

(The best way to know is to try and build it, and the best way to do that is probably having an actual Windows enlistment, and.. well, you know.)

DHowett · 2022-04-13T19:22:52Z

I was hoping to try and address the issues that Leonard raised at the same time

I'm happy to wait for this! Thanks!

DHowett

I am so on board with this. I love the reduced indirection and the improved obviousness of what adaptDispatch is doing. Thanks so much, James.

I'm only marking a comment here so that we don't automerge, but... this is an approval through and through

DHowett · 2022-04-13T22:03:55Z

src/terminal/adapter/adaptDispatch.cpp

-    if (savedCursorState.IsOriginModeRelative && _scrollMargins.Bottom != 0)
+    // If the origin mode is relative, we need to make sure the restored
+    // position is clamped within the margins.
+    if (savedCursorState.IsOriginModeRelative)


I'm curious why we don't need to check any longer that there are margins -- what overarching change made this safe to do both with and without margins?

Now we have the _GetVerticalMargins helper function which always returns a valid set of margins - if margins haven't been set, it'll return the margins of the viewport itself. In this particular case it means we'd be clamping the row to the viewport, which is essentially a noop. But in general it makes things cleaner not having to care whether the margins are set or not.

Ah, right -- you called this out in the PR body. Thanks!

DHowett · 2022-04-13T22:04:52Z

src/terminal/adapter/adaptDispatch.cpp

-    // This uses a private API instead of the public one, because the public API
-    //      will set the cursor shape back to legacy.
-    _pConApi->SetCursorVisibility(fIsVisible);
+    _pConApi->GetTextBuffer().GetCursor().SetIsVisible(fIsVisible);


Might be good to ruminate on what happens if we ever make the Cursor a "global good" (since we learned that an alt buffer and main buffer share cursor properties, for example -- does it make sense to have one per text buffer then?)

The problem is that the cursor coordinates are absolute, so when the buffers aren't the same size, the coordinates won't be the same. And when it comes to the console buffers, I think their cursor positions might be completely independent - I can't remember now.

Cursor properties (like visibility, shape, blinking status) could possibly be made global though. I've made a few attempts at that in the past, but always got stuck on something. I haven't given up on the idea, but it wasn't as straightforward as I expected it to be.

That totally makes sense. I legitimately forgot that the cursor's position wasn't shared.

ghost · 2022-04-13T23:37:08Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2022-04-13T23:43:13Z

Bot is going crazy. I'll fix that.

@msftbot forget everything i just told you

DHowett · 2022-04-13T23:43:56Z

@msftbot nevermind

ghost · 2022-04-13T23:44:00Z

Hello @DHowett!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

## Summary of the Pull Request This PR replaces the `TerminalDispatch` class with the `AdaptDispatch` class from conhost, so we're no longer duplicating the VT functionality in two places. It also gives us a more complete VT implementation on the Terminal side, so it should work better in pass-through mode. ## References This is essentially part two of PR #12703. ## PR Checklist * [x] Closes #3849 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12662 ## Detailed Description of the Pull Request / Additional comments The first thing was to give the `ConGetSet` interface a new name, since it's now no longer specific to conhost. I went with `ITerminalApi`, since that was the equivalent interface on the terminal side, and it still seemed like a generic enough name. I also changed the way the api is managed by the `AdaptDispatch` class, so it's now stored as a reference rather than a `unique_ptr`, which more closely matches the way the `TerminalDispatch` class worked. I then had to make sure that `AdaptDispatch` actually included all of the functionality currently in `TerminalDispatch`. That meant copying across the code for bracketed paste mode, the copy to clipboard operation, and the various ConEmu OSC operations. This also required a few new methods to the `ConGetSet`/`ITerminalApi` interface, but for now these are just stubs in conhost. Then there were a few thing in the api interface that needed cleaning up. The `ReparentWindow` method doesn't belong there, so I've moved that into `PtySignalInputThread` class. And the `WriteInput` method was too low-level for the Terminal requirements, so I've replaced that with a `ReturnResponse` method which takes a `wstring_view`. It was then a matter of getting the `Terminal` class to implement all the methods in the new `ITerminalApi` interface that it didn't already have. This was mostly mapping to existing functionality, but there are still a number of methods that I've had to leave as stubs for now. However, what we have is still good enough that I could then nuke the `TerminalDispatch` class from the Terminal code and replace it with `AdaptDispatch`. One oddity that came up in testing, though, was the `AdaptDispatch` implementation of `EraseAll` would push a blank line into the scrollback when called on an empty buffer, whereas the previous terminal implementation did not. That caused problems for the conpty connection, because one of the first things it does on startup is send an `ED 2` sequence. I've now updated the `AdaptDispatch` implementation to match the behavior of the terminal implementation in that regard. Another problem was that the terminal implementation of the color table commands had special handling for the background color to notify the application window that it needed to repaint the background. I didn't want to have to push the color table operations through the `ITerminalApi` interface, so I've instead moved the handling of the background update into the renderer, initiated by a flag on the `TriggerRefreshAll` method. ## Validation Steps Performed Surprisingly this PR didn't require a lot of changes to get the unit tests working again. There were just a few methods used from the original `ITerminalApi` that have now been removed, and which needed an equivalent replacement. Also the updated behavior of the `EraseAll` method in conhost resulted in a change to the expected cursor position in one of the screen buffer tests. In terms of manual testing, I've tried out all the different shells in Windows Terminal to make sure there wasn't anything obviously wrong. And I've run a bunch of the tests from _vttest_ to try and get a wider coverage of the VT functionality, and confirmed everything still works at least as well as it used to. I've also run some of my own tests to verify the operations that had to be copied from `TerminalDispatch` to `AdaptDispatch`.

## Summary of the Pull Request When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression. ## References The conpty passthrough mode was introduced in PR #11264. The refactoring that broke the cursor position handling was in PR #12703. ## PR Checklist * [x] Closes #13106 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class. After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required). However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged. So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`. This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before. ## Validation Steps Performed I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell.

## Summary of the Pull Request When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression. ## References The conpty passthrough mode was introduced in PR #11264. The refactoring that broke the cursor position handling was in PR #12703. ## PR Checklist * [x] Closes #13106 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class. After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required). However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged. So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`. This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before. ## Validation Steps Performed I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell. (cherry picked from commit e1086de) Service-Card-Id: 82005205 Service-Version: 1.14

ghost · 2022-05-24T17:04:03Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

j4james force-pushed the refactor-congetset-2 branch from ddd3a73 to 42d3d34 Compare March 21, 2022 00:48

j4james marked this pull request as ready for review March 22, 2022 01:03

zadjii-msft reviewed Mar 29, 2022

View reviewed changes

j4james added 19 commits April 10, 2022 19:23

Add GetTextBuffer and GetViewport to ConGetSet interface.

4b8ff8f

Replace most GetConsoleScreenBufferInfoEx usage.

6f5b9d3

Make more use of GetTextBuffer where possible.

e449c64

Remove unused ConGetSet methods.

981075d

Move IL/DL implementation to AdaptDispatch.

93fee50

Reimplement RI in AdaptDispatch.

20ab0c4

Remove unused SCREEN_INFORMATION margin methods.

6a0744b

Reimplement DECCOLM using ResizeWindow.

177f5ec

Reimplement EraseAll in AdaptDispatch.

ae439db

Replace SetWindowInfo with SetViewportPosition.

23275ab

Remove SuppressResizeRepaint from ConGetSet.

01df805

Reimplement SetInputMode in AdaptDispatch.

dcee135

Remove WriteControlInput from ConGetSet.

fdd9368

Remove ClearBuffer from ConGetSet.

9f78c07

Move Get/SetParserMode to AdaptDispatch.

f1f7bcf

Remove RefreshWindow from ConGetSet.

51e7b2d

Reimplement render ops in AdaptDispatch.

05e286c

Reimplement SetCursorPosition in AdaptDispatch.

f2869e7

Reimplement FillRegion in AdaptDispatch.

04ff8e3

zadjii-msft approved these changes Apr 13, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 13, 2022

DHowett reviewed Apr 13, 2022

View reviewed changes

j4james added 3 commits April 13, 2022 23:23

Pass til::rect by reference.

036345f

Remove unnecessary IsActiveScreenBuffer test.

79d6a5d

Merge branch 'main' into refactor-congetset-2

4eb7197

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 13, 2022

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 13, 2022

DHowett approved these changes Apr 13, 2022

View reviewed changes

ghost merged commit 7f5caa1 into microsoft:main Apr 14, 2022

This was referenced Apr 26, 2022

Prevent the virtual viewport bottom being updated incorrectly #12972

Merged

Merge the TerminalDispatch and AdaptDispatch classes #13024

Merged

j4james deleted the refactor-congetset-2 branch May 14, 2022 12:56

This was referenced May 15, 2022

Passthrough mode's input handling appears to have regressed #13106

Closed

Fix handling of cursor position reports in passthrough mode #13109

Merged

ghost mentioned this pull request May 24, 2022

Further simplification of the ConGetSet interface #12662

Closed

This pull request was closed.

	const auto data = OutputCell(*screenInfo.GetCellDataAt(sourcePos));
	screenInfo.Write(OutputCellIterator({ &data, 1 }), targetPos);

	source.WalkInBounds(sourcePos, walkDirection);
	} while (target.WalkInBounds(targetPos, walkDirection));

	if (gci.Flags & CONSOLE_HAS_FOCUS)
	{
	if (TurnOn)
	{
	cursor.SetDelay(false);
	cursor.SetIsOn(true);
	}
	else
	{
	cursor.SetDelay(true);
	}
	cursor.SetHasMoved(true);
	}

	if (!WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))
	{
	goto DoScroll;
	}

Further refactor and simplify the ConGetSet API #12703

Further refactor and simplify the ConGetSet API #12703

Conversation

j4james commented Mar 15, 2022 • edited by DHowett Loading

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

j4james commented Mar 22, 2022

lhecker commented Mar 23, 2022 • edited Loading

zadjii-msft commented Mar 29, 2022

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Mar 29, 2022 • edited Loading

j4james commented Mar 29, 2022

zadjii-msft commented Apr 13, 2022

DHowett commented Apr 13, 2022

ghost commented Apr 13, 2022

j4james commented Apr 13, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Apr 13, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 13, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett commented Apr 13, 2022

DHowett commented Apr 13, 2022

ghost commented Apr 13, 2022

ghost commented May 24, 2022

j4james commented Mar 15, 2022 •

edited by DHowett

Loading

lhecker commented Mar 23, 2022 •

edited

Loading

zadjii-msft commented Mar 29, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.