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

Fix session restoration of full buffers #17654

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Aug 2, 2024

This removes the Terminal::SetViewportPosition call from session
restoration which was responsible for putting the viewport below
the buffer height and caused the renderer to fail.

In order to prevent such issues in the future, SetViewportPosition
now protects itself against out of bounds requests.

Closes #17639

Validation Steps Performed

  • Enable persistence
  • Print big.txt
  • Restart
  • Looks good ✅

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Aug 2, 2024
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.

IMO the outcome should be that H-4 lines of the reloaded buffer get pushed off the top of scrollback and lost; what does this change result in?

@lhecker
Copy link
Member Author

lhecker commented Aug 2, 2024

Oh, good point. I should not move the viewport but rather print newlines and then CUP up.

Edit: No wait, we can't, because once we persist the buffer accurately, including trailing whitespace, those newlines will also get persisted. It must be viewport panning! Hmmmm... Perhaps we have to change the renderer to support out of bounds viewports. But that may be a future improvement.

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.

(Lodging my review)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 7, 2024
@lhecker lhecker marked this pull request as draft August 8, 2024 17:49
@lhecker
Copy link
Member Author

lhecker commented Aug 8, 2024

We talked about it and decided to just show as much scrollback as possible, since the popups in cmd now use VT.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 8, 2024
@lhecker lhecker force-pushed the dev/lhecker/17639-restore-viewport branch from 0d78d84 to e7527ac Compare August 9, 2024 16:19
@lhecker lhecker marked this pull request as ready for review August 9, 2024 16:20
{
_terminal->Write(L"\r\n");
}
_terminal->Write(message);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moving it into the loop allows us to reuse the console lock that we've already acquired.

@DHowett DHowett merged commit 9074e9d into main Aug 13, 2024
20 checks passed
@DHowett DHowett deleted the dev/lhecker/17639-restore-viewport branch August 13, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restoring a session as tall as the buffer breaks *everything*
3 participants