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

Make all console output modes more strictly buffer state #14735

Merged
6 commits merged into from
Jan 27, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jan 26, 2023

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.

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.

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.

Closes #14690

@ghost ghost added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Jan 26, 2023
@j4james j4james marked this pull request as ready for review January 26, 2023 11:45
@j4james
Copy link
Collaborator Author

j4james commented Jan 26, 2023

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.

@DHowett
Copy link
Member

DHowett commented Jan 27, 2023

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.

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.

Approved with comment - I expect that I've missed something, and there's a second reviewer yet required. 😄


if (ServiceLocator::LocateGlobals().pRender != nullptr)
{
ServiceLocator::LocateGlobals().pRender->TriggerRedrawAll();
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to retain this logic? We might miss a global repaint if we don't, and grid lines that had been written will not appear on demand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This redraw was also being done in the SetConsoleOutputModeImpl method, so this code was never really necessary. See here:

// if we changed rendering modes then redraw the output buffer,
// but only do this if we're not in conpty mode.
if (!gci.IsInVtIoMode() &&
(WI_IsFlagSet(dwNewMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING) != WI_IsFlagSet(dwOldMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING) ||
WI_IsFlagSet(dwNewMode, ENABLE_LVB_GRID_WORLDWIDE) != WI_IsFlagSet(dwOldMode, ENABLE_LVB_GRID_WORLDWIDE)))
{
auto* pRender = ServiceLocator::LocateGlobals().pRender;
if (pRender)
{
pRender->TriggerRedrawAll();
}
}

I should also mention that I wrote some little test apps similar to the functional tests, but which render output to visually confirm the modes are working as expected. And when the ENABLE_LVB_GRID_WORLDWIDE mode is toggled, the screen was definitely being refreshed, with the grid line disappearing and reappearing.

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Love the changeset. But @DHowett makes a valid point about the renderer invalidation. I don't know what ENABLE_LVB_GRID_WORLDWIDE does (I've never tried it), but I imagine that it requires an invalidation when the setting changes.

@DHowett
Copy link
Member

DHowett commented Jan 27, 2023

That's all I need. Thank you!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 27, 2023
@ghost
Copy link

ghost commented Jan 27, 2023

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.

@ghost ghost merged commit 282c583 into microsoft:main Jan 27, 2023
@j4james
Copy link
Collaborator Author

j4james commented Jan 27, 2023

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 MicrosoftDocs/Console-Docs#276 (comment)) 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.

@DHowett
Copy link
Member

DHowett commented Feb 10, 2023

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.

@j4james j4james deleted the refactor-output-modes branch March 21, 2023 11:14
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wslsys in alt buffer toggles DISABLE_NEWLINE_AUTO_RETURN
3 participants