Skip to content

Commit

Permalink
Make all console output modes more strictly buffer state (#14735)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
j4james authored Jan 27, 2023
1 parent fc960e3 commit 282c583
Show file tree
Hide file tree
Showing 11 changed files with 77 additions and 69 deletions.
2 changes: 1 addition & 1 deletion src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
// move cursor to the next line.
pwchBuffer++;

if (gci.IsReturnOnNewlineAutomatic())
if (WI_IsFlagClear(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN))
{
// Traditionally, we reset the X position to 0 with a newline automatically.
// Some things might not want this automatic "ONLCR line discipline" (for example, things that are expecting a *NIX behavior.)
Expand Down
50 changes: 50 additions & 0 deletions src/host/ft_host/API_ModeTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ModeTests

TEST_METHOD(TestConsoleModeInputScenario);
TEST_METHOD(TestConsoleModeScreenBufferScenario);
TEST_METHOD(TestConsoleModeAcrossMultipleBuffers);

TEST_METHOD(TestGetConsoleDisplayMode);

Expand Down Expand Up @@ -94,6 +95,55 @@ void ModeTests::TestConsoleModeScreenBufferScenario()
VERIFY_ARE_EQUAL(dwOutputMode, (DWORD)0, L"Verify able to set zero output flags");
}

void ModeTests::TestConsoleModeAcrossMultipleBuffers()
{
auto dwInitialMode = (DWORD)-1;
VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(Common::_hConsole, &dwInitialMode),
L"Get initial output flags");

Log::Comment(L"Verify initial flags match the expected defaults");
VERIFY_IS_TRUE(WI_IsFlagSet(dwInitialMode, ENABLE_PROCESSED_OUTPUT));
VERIFY_IS_TRUE(WI_IsFlagSet(dwInitialMode, ENABLE_WRAP_AT_EOL_OUTPUT));
VERIFY_IS_TRUE(WI_IsFlagClear(dwInitialMode, DISABLE_NEWLINE_AUTO_RETURN));
VERIFY_IS_TRUE(WI_IsFlagClear(dwInitialMode, ENABLE_LVB_GRID_WORLDWIDE));

// The initial VT flag may vary, dependent on the VirtualTerminalLevel registry entry.
const auto defaultVtFlag = WI_IsFlagSet(dwInitialMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);

auto dwUpdatedMode = dwInitialMode;
WI_ClearFlag(dwUpdatedMode, ENABLE_PROCESSED_OUTPUT);
WI_ClearFlag(dwUpdatedMode, ENABLE_WRAP_AT_EOL_OUTPUT);
WI_SetFlag(dwUpdatedMode, DISABLE_NEWLINE_AUTO_RETURN);
WI_SetFlag(dwUpdatedMode, ENABLE_LVB_GRID_WORLDWIDE);
WI_UpdateFlag(dwUpdatedMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING, !defaultVtFlag);
VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(Common::_hConsole, dwUpdatedMode),
L"Update flags to the opposite of their initial values");

auto hSecondBuffer = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE,
0 /*dwShareMode*/,
nullptr /*lpSecurityAttributes*/,
CONSOLE_TEXTMODE_BUFFER,
nullptr /*lpReserved*/);
VERIFY_ARE_NOT_EQUAL(INVALID_HANDLE_VALUE, hSecondBuffer, L"Create a second screen buffer");

auto dwSecondBufferMode = (DWORD)-1;
VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(hSecondBuffer, &dwSecondBufferMode),
L"Get initial flags for second buffer");

VERIFY_ARE_EQUAL(dwInitialMode, dwSecondBufferMode, L"Verify second buffer initialized with defaults");

VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(hSecondBuffer, dwSecondBufferMode),
L"Reapply mode to test if it affects the main buffer");

VERIFY_WIN32_BOOL_SUCCEEDED(CloseHandle(hSecondBuffer), L"Close the second buffer");

auto dwFinalMode = (DWORD)-1;
VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(Common::_hConsole, &dwFinalMode),
L"Get flags from the main buffer again");

VERIFY_ARE_EQUAL(dwUpdatedMode, dwFinalMode, L"Verify main buffer flags haven't changed");
}

void ModeTests::TestGetConsoleDisplayMode()
{
DWORD dwMode = 0;
Expand Down
4 changes: 0 additions & 4 deletions src/host/getset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -436,10 +436,6 @@ void ApiRoutines::GetNumberOfConsoleMouseButtonsImpl(ULONG& buttons) noexcept
screenInfo.GetStateMachine().ResetState();
}

gci.SetVirtTermLevel(WI_IsFlagSet(dwNewMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING) ? 1 : 0);
gci.SetAutomaticReturnOnNewline(WI_IsFlagSet(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN) ? false : true);
gci.SetGridRenderingAllowedWorldwide(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_LVB_GRID_WORLDWIDE));

// if we changed rendering modes then redraw the output buffer,
// but only do this if we're not in conpty mode.
if (!gci.IsInVtIoMode() &&
Expand Down
4 changes: 2 additions & 2 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ void ConhostInternalGetSet::SetScrollingRegion(const til::inclusive_rect& scroll
// - true if a line feed also produces a carriage return. false otherwise.
bool ConhostInternalGetSet::GetLineFeedMode() const
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
return gci.IsReturnOnNewlineAutomatic();
auto& screenInfo = _io.GetActiveOutputBuffer();
return WI_IsFlagClear(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN);
}

// Routine Description:
Expand Down
24 changes: 9 additions & 15 deletions src/host/renderData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,26 +261,20 @@ bool RenderData::IsCursorDoubleWidth() const noexcept
const bool RenderData::IsGridLineDrawingAllowed() noexcept
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
// If virtual terminal output is set, grid line drawing is a must. It is always allowed.
if (WI_IsFlagSet(gci.GetActiveOutputBuffer().OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING))
const auto outputMode = gci.GetActiveOutputBuffer().OutputMode;
// If virtual terminal output is set, grid line drawing is a must. It is also enabled
// if someone explicitly asked for worldwide line drawing.
if (WI_IsAnyFlagSet(outputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_LVB_GRID_WORLDWIDE))
{
return true;
}
else
{
// If someone explicitly asked for worldwide line drawing, enable it.
if (gci.IsGridRenderingAllowedWorldwide())
{
return true;
}
else
{
// Otherwise, for compatibility reasons with legacy applications that used the additional CHAR_INFO bits by accident or for their own purposes,
// we must enable grid line drawing only in a DBCS output codepage. (Line drawing historically only worked in DBCS codepages.)
// The only known instance of this is Image for Windows by TeraByte, Inc. (TeraByte Unlimited) which used the bits accidentally and for no purpose
// (according to the app developer) in conjunction with the Borland Turbo C cgscrn library.
return !!IsAvailableEastAsianCodePage(gci.OutputCP);
}
// Otherwise, for compatibility reasons with legacy applications that used the additional CHAR_INFO bits by accident or for their own purposes,
// we must enable grid line drawing only in a DBCS output codepage. (Line drawing historically only worked in DBCS codepages.)
// The only known instance of this is Image for Windows by TeraByte, Inc. (TeraByte Unlimited) which used the bits accidentally and for no purpose
// (according to the app developer) in conjunction with the Borland Turbo C cgscrn library.
return !!IsAvailableEastAsianCodePage(gci.OutputCP);
}
}

Expand Down
12 changes: 9 additions & 3 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ SCREEN_INFORMATION::SCREEN_INFORMATION(
_desiredFont{ fontInfo },
_ignoreLegacyEquivalentVTAttributes{ false }
{
// Check if VT mode is enabled. Note that this can be true w/o calling
// SetConsoleMode, if VirtualTerminalLevel is set to !=0 in the registry.
// Check if VT mode should be enabled by default. This can be true if
// VirtualTerminalLevel is set to !=0 in the registry, or when conhost
// is started in conpty mode.
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
if (gci.GetVirtTermLevel() != 0)
if (gci.GetDefaultVirtTermLevel() != 0)
{
OutputMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
}
Expand Down Expand Up @@ -1913,6 +1914,8 @@ const SCREEN_INFORMATION& SCREEN_INFORMATION::GetMainBuffer() const
auto altCursorPos = myCursor.GetPosition();
altCursorPos.y -= GetVirtualViewport().Top();
altCursor.SetPosition(altCursorPos);
// The alt buffer's output mode should match the main buffer.
createdBuffer->OutputMode = OutputMode;

s_InsertScreenBuffer(createdBuffer);

Expand Down Expand Up @@ -2078,6 +2081,9 @@ void SCREEN_INFORMATION::UseMainScreenBuffer()
mainCursor.SetIsVisible(altCursor.IsVisible());
mainCursor.SetBlinkingAllowed(altCursor.IsBlinkingAllowed());

// Copy the alt buffer's output mode back to the main buffer.
psiMain->OutputMode = psiAlt->OutputMode;

s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer
// deleting the alt buffer will give the GetSet back to its main

Expand Down
33 changes: 2 additions & 31 deletions src/host/settings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ Settings::Settings() :
_fAllowAltF4Close(true),
_dwVirtTermLevel(0),
_fUseWindowSizePixels(false),
_fAutoReturnOnNewline(true), // the historic Windows behavior defaults this to on.
_fRenderGridWorldwide(false), // historically grid lines were only rendered in DBCS codepages, so this is false by default unless otherwise specified.
// window size pixels initialized below
_fInterceptCopyPaste(0),
_fUseDx(UseDx::Disabled),
Expand Down Expand Up @@ -358,11 +356,11 @@ void Settings::Validate()
FAIL_FAST_IF(!(_dwScreenBufferSize.Y > 0));
}

DWORD Settings::GetVirtTermLevel() const
DWORD Settings::GetDefaultVirtTermLevel() const
{
return _dwVirtTermLevel;
}
void Settings::SetVirtTermLevel(const DWORD dwVirtTermLevel)
void Settings::SetDefaultVirtTermLevel(const DWORD dwVirtTermLevel)
{
_dwVirtTermLevel = dwVirtTermLevel;
}
Expand All @@ -376,33 +374,6 @@ void Settings::SetAltF4CloseAllowed(const bool fAllowAltF4Close)
_fAllowAltF4Close = fAllowAltF4Close;
}

bool Settings::IsReturnOnNewlineAutomatic() const
{
return _fAutoReturnOnNewline;
}
void Settings::SetAutomaticReturnOnNewline(const bool fAutoReturnOnNewline)
{
_fAutoReturnOnNewline = fAutoReturnOnNewline;
}

bool Settings::IsGridRenderingAllowedWorldwide() const
{
return _fRenderGridWorldwide;
}
void Settings::SetGridRenderingAllowedWorldwide(const bool fGridRenderingAllowed)
{
// Only trigger a notification and update the status if something has changed.
if (_fRenderGridWorldwide != fGridRenderingAllowed)
{
_fRenderGridWorldwide = fGridRenderingAllowed;

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

bool Settings::GetFilterOnPaste() const
{
return _fFilterOnPaste;
Expand Down
12 changes: 2 additions & 10 deletions src/host/settings.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,12 @@ class Settings
RenderSettings& GetRenderSettings() noexcept { return _renderSettings; };
const RenderSettings& GetRenderSettings() const noexcept { return _renderSettings; };

DWORD GetVirtTermLevel() const;
void SetVirtTermLevel(const DWORD dwVirtTermLevel);
DWORD GetDefaultVirtTermLevel() const;
void SetDefaultVirtTermLevel(const DWORD dwVirtTermLevel);

bool IsAltF4CloseAllowed() const;
void SetAltF4CloseAllowed(const bool fAllowAltF4Close);

bool IsReturnOnNewlineAutomatic() const;
void SetAutomaticReturnOnNewline(const bool fAutoReturnOnNewline);

bool IsGridRenderingAllowedWorldwide() const;
void SetGridRenderingAllowedWorldwide(const bool fGridRenderingAllowed);

bool GetFilterOnPaste() const;
void SetFilterOnPaste(const bool fFilterOnPaste);

Expand Down Expand Up @@ -225,8 +219,6 @@ class Settings
std::wstring _LaunchFaceName;
bool _fAllowAltF4Close;
DWORD _dwVirtTermLevel;
bool _fAutoReturnOnNewline;
bool _fRenderGridWorldwide;
UseDx _fUseDx;
bool _fCopyColor;

Expand Down
2 changes: 1 addition & 1 deletion src/host/srvinit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ static bool s_IsOnDesktop()
// We want everyone to be using VT by default anyways, so this is a
// strong nudge in that direction. If an application _doesn't_ want VT
// processing, it's free to disable this setting, even in conpty mode.
settings.SetVirtTermLevel(1);
settings.SetDefaultVirtTermLevel(1);

// GH#9458 - In the case of a DefTerm handoff, the OriginalTitle might
// be stashed in the lnk. We want to crack that lnk open, so we can get
Expand Down
2 changes: 1 addition & 1 deletion src/host/telemetry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ void Telemetry::WriteFinalTraceLog()
TraceLoggingValue(gci.GetScreenBufferSize().width, "ScreenBufferSizeX"),
TraceLoggingValue(gci.GetScreenBufferSize().height, "ScreenBufferSizeY"),
TraceLoggingValue(gci.GetStartupFlags(), "StartupFlags"),
TraceLoggingValue(gci.GetVirtTermLevel(), "VirtualTerminalLevel"),
TraceLoggingValue(gci.GetDefaultVirtTermLevel(), "VirtualTerminalLevel"),
TraceLoggingValue(gci.GetWindowSize().width, "WindowSizeX"),
TraceLoggingValue(gci.GetWindowSize().height, "WindowSizeY"),
TraceLoggingValue(gci.GetWindowOrigin().width, "WindowOriginX"),
Expand Down
1 change: 0 additions & 1 deletion src/host/ut_host/TextBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1537,7 +1537,6 @@ void TextBufferTests::TestBackspaceStringsAPI()
const auto& tbi = si.GetTextBuffer();
const auto& cursor = tbi.GetCursor();

gci.SetVirtTermLevel(0);
WI_ClearFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);

const auto x0 = cursor.GetPosition().x;
Expand Down

0 comments on commit 282c583

Please sign in to comment.