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

Don't push the buffer contents into the scrollback on session restore #17274

Closed
mmseng opened this issue May 16, 2024 · 4 comments · Fixed by #17334
Closed

Don't push the buffer contents into the scrollback on session restore #17274

mmseng opened this issue May 16, 2024 · 4 comments · Fixed by #17334
Assignees
Labels
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. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@mmseng
Copy link

mmseng commented May 16, 2024

Windows Terminal version

Windows Terminal Preview Version: 1.21.1272.0

Windows build number

10.0.19045.0

Other Software

No response

Summary

I installed Windows Terminal Preview to make use of the new buffer restoration feature, noted as the first bullet here: https://github.com/microsoft/terminal/releases/tag/v1.21.1272.0 (i.e. #16598), but it doesn't seem to behave any differently than previous versions, out of the box. Not sure if the feature is broken, or I'm doing something wrong, or what.

While searching related issues, I saw #17179, but it seems to be related more to special cases while restoring after a reboot due to Windows updates. In my case, I can't get the functionality to work even under the simplest condition of manually closing and reopening Terminal.

Steps to reproduce

  1. Install Windows Terminal Preview Version: 1.21.1272.0 from the MS app store.
  2. Launch Terminal Preview.
  3. Ensure that Settings -> Startup -> When Terminal starts is set to Open windows from a previous session.
  4. Run some commands in a tab. In my case I'm just testing with a PowerShell 7 tab with no profile customizations and running Get-Process.
  5. Close Terminal Preview.
  6. Launch Preview.

Expected Behavior

Original tabs, titles, and contents are restored. In the example case, the single PowerShell 7 tab should have the output of the previously-run Get-Process command displayed in the buffer content, along with however much further buffer history is expected by this feature.

Actual Behavior

Original tabs and tab titles are restored (as in previous versions), but buffer contents are empty (as in previous versions).

Additional info

According to the release notes linked above:

Buffer snapshots are stored in human-readable (albeit UTF-16) VT-encoded text in your package's local state directory, next to your settings.

I do see a file named buffer_<guid>.txt alongside my settings.json and state.json files in the Terminal Preview installation's LocalState folder (where such a file doesn't exist in the production Terminal app's LocalState folder). And that file does contain the output of Get-Process, as expected. But It's apparently just not being pulled into Terminal on a restart. I even see a couple lines in the buffer file that look like [Restored <DateTime>].

@mmseng mmseng added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 16, 2024
@zadjii-msft
Copy link
Member

So, this may sound silly, but did you scroll up after the buffer was restored/? The old buffer contents are restored into the scrollback above the new viewport

image

@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 May 16, 2024
@mmseng
Copy link
Author

mmseng commented May 16, 2024

Oh lord. You're right. It's there and I just never scrolled up 🤦

I suppose I was expecting the window to look exactly the same as the way I left it (i.e. at the same scrolling position).

Well I'm certainly happy that it works (been desperately wanting this feature for a long time). I guess my feedback is that I would prefer an option to additionally restore the scrolling position. However I recognize that that doesn't necessarily make sense (at least in the current implementation), because the restored buffer is not actually 1:1 with the buffer from before the restoration due to the injection of the [Restored <DateTime>] events, which are actually a nice feature IMO, as they clearly log when the console was restarted.

Thanks for your response.

@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 May 16, 2024
@lhecker
Copy link
Member

lhecker commented May 16, 2024

You're not the first person to bring this flaw up. We've talked about fixing the issue before, and I'm mostly leaning towards doing so now. However, this will increase the startup cost quite a bit, as the cursor position after session restoration needs to be synced up, etc.

@mmseng
Copy link
Author

mmseng commented May 16, 2024

I do think if that "fix" is added, it would be best as an optional setting (maybe even non-default), as I can definitely see people (perhaps even myself at times) preferring the current behavior. I'm guessing that would also mean people could opt into/out of the additional startup cost, maybe. Just my 2 cents.

@lhecker lhecker changed the title New buffer restore feature in 1.21.1272.0 not working? Don't push the buffer contents into the scrollback on session restore May 16, 2024
@lhecker lhecker self-assigned this May 16, 2024
github-merge-queue bot pushed a commit that referenced this issue May 30, 2024
First, this makes use of `PSEUDOCONSOLE_INHERIT_CURSOR` to stop ConPTY
from emitting a CSI 2 J on startup. Then, it uses
`Terminal::SetViewportPosition` to fake-scroll the viewport down so that
only 3 lines of scrollback are visible. It avoids printing actual
newlines because if we later change the text buffer to actually track
the written contents, we don't want those newlines to end up in the next
buffer snapshot.

Closes #17274

## Validation Steps Performed
* Restore cmd multiple times
* There's always exactly 3 lines visible ✅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants