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

Merge the LineFeed functionality into AdaptDispatch #14874

Merged
merged 14 commits into from
Mar 30, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Feb 18, 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 Scrolling within regions that include the top of the screen doesn't push lines into scrollback #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

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Feb 18, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Area-VT Virtual Terminal sequence support label Feb 21, 2023
@j4james j4james marked this pull request as ready for review February 23, 2023 12:25
@DHowett
Copy link
Member

DHowett commented Mar 27, 2023

I didn't notice in the PR description that margins also moved into AdaptDispatch! Very clever, I love the new better encapsulation.

// We trigger a scroll rather than a redraw, since that's more efficient,
// but we need to turn the cursor off before doing so, otherwise a ghost
// cursor can be left behind in the previous position.
cursor.SetIsOn(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: who turns it back on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a call to _ApplyCursorMovementFlags at the end of the method which turns the cursor back on.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all makes sense to me! I'd love to get @zadjii-msft's or @lhecker's hands on the ball as well.

Thanks for being so patient with us, @j4james!

@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone Mar 30, 2023
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for remedying the abomination that was AdjustCursorPosition - that got wildly out of hand during windows 10 and I absolutely hated every minute of it. This seems a lot easier to parse.

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-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrolling within regions that include the top of the screen doesn't push lines into scrollback
3 participants