terminal

terminal copied to clipboard

Summary of the Pull Request

The original console output modes were considered attributes of the buffer, while later additions were treated as global state, and yet both were accessed via the same buffer-based API. This could result in the reported modes being out of sync with the way the system was actually behaving, and a call to SetConsoleMode without updating anything could still trigger unpredictable changes in behavior.

This PR attempts to address that problem by making all modes part of the buffer state, and giving them predictable default values.

References and Relevant Issues

While this won't solve all the tmux layout-breaking issues in #6987, it does at least fix one case which was the result of an unexpected change in the DISABLE_NEWLINE_AUTO_RETURN mode.

Detailed Description of the Pull Request / Additional comments

All access to the output modes is now done via the OutputMode field in SCREEN_INFORMATION. The fields that were tracking global state in the Settings class (_fAutoReturnOnNewline and _fRenderGridWorldwide) have now been removed.

We still have a global _dwVirtTermLevel field, though, but that now serves as a default value for the ENABLE_VIRTUAL_TERMINAL_PROCESSING mode when creating a new buffer. It's enabled for conpty mode, and when the VT level in the registry is not 0. That default doesn't change.

For the VT alternate buffer, things works slightly differently, since there is an expectation that VT modes are global. So when creating an alt buffer, we copy the current modes from the main buffer, and when it's closed, we copy them back again.

Validation Steps Performed

I've manually confirmed that this fixes the problem described in issue #14690. I've also added a basic feature test that confirms the modes are initialized as expected when creating new buffers, and changes to the modes in one buffer do not impact any other buffers.

PR Checklist

• [x] Closes #14690
• [x] Tests added/passed
• [ ] Documentation updated
• [ ] Schema updated (if necessary)

I think this is logically the right approach to take, in that the "rules" are straightforward, so the behavior is easily predictable. However, my one concern is with the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag - it's possible that there are apps creating multiple console buffers that are also setting the VT mode, and they may expect that they can set it once and have that change "inherited" when creating additional buffers. I don't know if that's a likely scenario though.

So when creating an alt buffer, we copy the current modes from the main buffer, and when it's closed, we copy them back again.

Totally cool with this. Alt buffer being a SCREEN_INFORMATION is an implementation detail, not a contract, and doing this simply pays down a debt we incurred by using that implementation detail.

Thanks for doing this.

That's all I need. Thank you!

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.

I know it's a bit late now, since we're merged, but just to be clear, my concern regarding the ENABLE_VIRTUAL_TERMINAL_PROCESSING mode is not with the alt buffer, but with people that are using CreateConsoleScreenBuffer to create additional buffers. There was a comment in the Console-Docs issue tracker (see https://github.com/MicrosoftDocs/Console-Docs/issues/276#issuecomment-1217248088) which made me realise that some people might be expecting that mode to be inherited.

Technically the current behavior isn't really inheritance anyway, but it's probably close enough for most use cases that are expecting that. But after this PR, you'll need to explicitly set the mode in every buffer you create (not counting the alt buffer). That's assuming you want it changed from the default, i.e. you want to turn it on in cohost, or turn it off in Windows Terminal.

Now I wouldn't have thought there were a lot of apps using CreateConsoleScreenBuffer that were also messing with the VT mode, and for those that are, I would hope they're setting the mode explicitly in every buffer anyway (you would already have had to do that for the original modes). But for anyone that isn't doing that, this is potentially a breaking change.

potentially breaking change

Two weeks ago, I took a note to document this breaking change somewhere. I finally did that here: https://github.com/microsoft/terminal/wiki/Console:-Potential-Breaking-Changes

I'm going to try to make sure we track this stuff a little better, because the Windows release cycle has gotten somewhat longer and it means we accumulate more things like this before people see them.