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

Improve the DECSC/DECRC implementation #3160

Merged
merged 8 commits into from
Nov 5, 2019
25 changes: 25 additions & 0 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,31 @@ BOOL ConhostInternalGetSet::PrivateSetExtendedTextAttributes(const ExtendedAttri
return TRUE;
}

// Method Description:
// - Retrieves the current TextAttribute of the active screen buffer.
// Arguments:
// - pAttrs: Receives the TextAttribute value.
// Return Value:
// - TRUE if successful. FALSE otherwise.
BOOL ConhostInternalGetSet::PrivateGetTextAttributes(TextAttribute* const pAttrs) const
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this change was inevitable. We've kinda just been patching around not just exposing the entire TextAttribute through this layer, but this might be easier. I know there was previously reluctance to doing this, something about it exposing too much of the implementation details of how conhost itself was implemented. @miniksa might remember more. I think we've come a long way since then, and this might be reasonable nowadays.

I guess from a design perspective, it would change things pretty dramatically - because the adapter could directly control the state of the text attributes, it could just avoid using any of the other functions, and put all the combined attribute manipulation logic directly in the adapter. There might be other things that conhost needs to do during some of these calls that might get circumnavigated by just using this method. These things might be a little trickier to move up into the adapter. I haven't dug in on this too much, so it might not be a problem.

If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.

The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.

Absolutely! I had exactly the same thought when I added this. There's a lot of weirdness in the current SGR implementation which I think could be greatly simplified if we had access to an API like this. Was going to raise a separate issue for that once I knew you were happy with the API and I had some time to play around with potential implementations.

The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...

If there was a need for conhost and Terminal to handle this functionality differently, that's already not a problem, since Terminal shares none of the AdaptDispatch implementation. That said, I'm not sure that's the right approach in the long term, because it seems we're slowly reimplementing all of the AdaptDispatch code in TerminalDispatch when those implementations should really be identical 99% of the time. But that's a topic for another issue.

Copy link
Member

Choose a reason for hiding this comment

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

If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.

Absolutely! I had exactly the same thought when I added this. There's a lot of weirdness in the current SGR implementation which I think could be greatly simplified if we had access to an API like this. Was going to raise a separate issue for that once I knew you were happy with the API and I had some time to play around with potential implementations.

I'm OK with additional private methods being added here, I just don't want to expose any more publicly. @zadjii-msft, I'm not sure I had an extreme reticence to adding new private APIs given we've done a ton of them. I think it was more "I'm going to try to accomplish as much as possible without adding new APIs at all" and it led us to the non-ideal situations that we had and discovered over time.

The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...

If there was a need for conhost and Terminal to handle this functionality differently, that's already not a problem, since Terminal shares none of the AdaptDispatch implementation. That said, I'm not sure that's the right approach in the long term, because it seems we're slowly reimplementing all of the AdaptDispatch code in TerminalDispatch when those implementations should really be identical 99% of the time. But that's a topic for another issue.

Yes, if we're duplicating a big portion of the dispatch... then we probably need to somehow abstract the buffers or whatever the two adapters are calling and make that a shared component. Feel free to file another issue for that if you feel you can articulate it.

{
*pAttrs = _io.GetActiveOutputBuffer().GetAttributes();
return TRUE;
}

// Method Description:
// - Sets the current TextAttribute of the active screen buffer. Text
// written to this buffer will be written with these attributes.
// Arguments:
// - attrs: The new TextAttribute to use
// Return Value:
// - TRUE if successful. FALSE otherwise.
BOOL ConhostInternalGetSet::PrivateSetTextAttributes(const TextAttribute& attrs)
{
_io.GetActiveOutputBuffer().SetAttributes(attrs);
return TRUE;
}

// Routine Description:
// - Connects the WriteConsoleInput API call directly into our Driver Message servicing call inside Conhost.exe
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/host/outputStream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal::
BOOL PrivateBoldText(const bool bolded) override;
BOOL PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) override;
BOOL PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) override;
BOOL PrivateGetTextAttributes(TextAttribute* const pAttrs) const override;
BOOL PrivateSetTextAttributes(const TextAttribute& attrs) override;

BOOL PrivateWriteConsoleInputW(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& events,
_Out_ size_t& eventsWritten) override;
Expand Down
122 changes: 121 additions & 1 deletion src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"
Expand Down Expand Up @@ -193,6 +193,8 @@ class ScreenBufferTests
TEST_METHOD(CursorUpDownAcrossMargins);
TEST_METHOD(CursorUpDownOutsideMargins);
TEST_METHOD(CursorUpDownExactlyAtMargins);

TEST_METHOD(CursorSaveRestore);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -5045,3 +5047,121 @@ void ScreenBufferTests::CursorUpDownExactlyAtMargins()

stateMachine.ProcessString(L"\x1b[r");
}

void ScreenBufferTests::CursorSaveRestore()
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
auto& cursor = si.GetTextBuffer().GetCursor();

const auto defaultAttrs = TextAttribute{};
const auto colorAttrs = TextAttribute{ RGB(12, 34, 56), RGB(78, 90, 12) };

const auto asciiText = L"lwkmvj";
const auto graphicText = L"┌┬┐└┴┘";

const auto selectAsciiChars = L"\x1b(B";
const auto selectGraphicsChars = L"\x1b(0";
const auto saveCursor = L"\x1b[s";
const auto restoreCursor = L"\x1b[u";
const auto setDECOM = L"\x1b[?6h";
const auto resetDECOM = L"\x1b[?6l";

Log::Comment(L"Make sure the viewport is at 0,0");
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));

Log::Comment(L"Restore after save.");
// Set the cursor position, attributes, and character set.
cursor.SetPosition(COORD{ 20, 10 });
si.SetAttributes(colorAttrs);
stateMachine.ProcessString(selectGraphicsChars);
// Save state.
stateMachine.ProcessString(saveCursor);
// Reset the cursor position, attributes, and character set.
cursor.SetPosition(COORD{ 0, 0 });
si.SetAttributes(defaultAttrs);
stateMachine.ProcessString(selectAsciiChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify initial position, colors, and graphic character set.
VERIFY_ARE_EQUAL(COORD({ 20, 10 }), cursor.GetPosition());
VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains(COORD({ 20, 10 }), graphicText, colorAttrs));

Log::Comment(L"Restore again without save.");
// Reset the cursor position, attributes, and character set.
cursor.SetPosition(COORD{ 0, 0 });
si.SetAttributes(defaultAttrs);
stateMachine.ProcessString(selectAsciiChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify initial saved position, colors, and graphic character set.
VERIFY_ARE_EQUAL(COORD({ 20, 10 }), cursor.GetPosition());
VERIFY_ARE_EQUAL(colorAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains(COORD({ 20, 10 }), graphicText, colorAttrs));

Log::Comment(L"Restore after reset.");
// Soft reset.
stateMachine.ProcessString(L"\x1b[!p");
// Set the cursor position, attributes, and character set.
cursor.SetPosition(COORD{ 20, 10 });
si.SetAttributes(colorAttrs);
stateMachine.ProcessString(selectGraphicsChars);
// Restore state.
stateMachine.ProcessString(restoreCursor);
// Verify home position, default attributes, and ascii character set.
VERIFY_ARE_EQUAL(COORD({ 0, 0 }), cursor.GetPosition());
VERIFY_ARE_EQUAL(defaultAttrs, si.GetAttributes());
stateMachine.ProcessString(asciiText);
VERIFY_IS_TRUE(_ValidateLineContains(COORD({ 0, 0 }), asciiText, defaultAttrs));

Log::Comment(L"Restore origin mode.");
// Set margins and origin mode to relative.
stateMachine.ProcessString(L"\x1b[10;20r");
stateMachine.ProcessString(setDECOM);
// Verify home position inside margins.
VERIFY_ARE_EQUAL(COORD({ 0, 9 }), cursor.GetPosition());
// Save state and reset origin mode to absolute.
stateMachine.ProcessString(saveCursor);
stateMachine.ProcessString(resetDECOM);
// Verify home position at origin.
VERIFY_ARE_EQUAL(COORD({ 0, 0 }), cursor.GetPosition());
// Restore state and move to home position.
stateMachine.ProcessString(restoreCursor);
stateMachine.ProcessString(L"\x1b[H");
// Verify home position inside margins, i.e. relative origin mode restored.
VERIFY_ARE_EQUAL(COORD({ 0, 9 }), cursor.GetPosition());

Log::Comment(L"Clamp inside top margin.");
// Reset margins, with absolute origin, and set cursor position.
stateMachine.ProcessString(L"\x1b[r");
stateMachine.ProcessString(setDECOM);
cursor.SetPosition(COORD{ 5, 15 });
// Save state.
stateMachine.ProcessString(saveCursor);
// Set margins and restore state.
stateMachine.ProcessString(L"\x1b[20;25r");
stateMachine.ProcessString(restoreCursor);
// Verfify Y position is clamped inside the top margin
VERIFY_ARE_EQUAL(COORD({ 5, 19 }), cursor.GetPosition());

Log::Comment(L"Clamp inside bottom margin.");
// Reset margins, with absolute origin, and set cursor position.
stateMachine.ProcessString(L"\x1b[r");
stateMachine.ProcessString(setDECOM);
cursor.SetPosition(COORD{ 5, 15 });
// Save state.
stateMachine.ProcessString(saveCursor);
// Set margins and restore state.
stateMachine.ProcessString(L"\x1b[1;10r");
stateMachine.ProcessString(restoreCursor);
// Verfify Y position is clamped inside the top margin
VERIFY_ARE_EQUAL(COORD({ 5, 9 }), cursor.GetPosition());

// Reset origin mode and margins.
stateMachine.ProcessString(resetDECOM);
stateMachine.ProcessString(L"\x1b[r");
}
4 changes: 2 additions & 2 deletions src/terminal/adapter/ITermDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
virtual bool CursorHorizontalPositionAbsolute(const unsigned int uiColumn) = 0; // CHA
virtual bool VerticalLinePositionAbsolute(const unsigned int uiLine) = 0; // VPA
virtual bool CursorPosition(const unsigned int uiLine, const unsigned int uiColumn) = 0; // CUP
virtual bool CursorSavePosition() = 0; // DECSC
virtual bool CursorRestorePosition() = 0; // DECRC
virtual bool CursorSaveState() = 0; // DECSC
virtual bool CursorRestoreState() = 0; // DECRC
virtual bool CursorVisibility(const bool fIsVisible) = 0; // DECTCEM
virtual bool InsertCharacter(const unsigned int uiCount) = 0; // ICH
virtual bool DeleteCharacter(const unsigned int uiCount) = 0; // DCH
Expand Down
76 changes: 58 additions & 18 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,14 @@ AdaptDispatch::AdaptDispatch(ConGetSet* const pConApi,
AdaptDefaults* const pDefaults) :
_conApi{ THROW_IF_NULL_ALLOC(pConApi) },
_pDefaults{ THROW_IF_NULL_ALLOC(pDefaults) },
_usingAltBuffer(false),
_fIsOriginModeRelative(false), // by default, the DECOM origin mode is absolute.
_fIsSavedOriginModeRelative(false), // as is the origin mode of the saved cursor position.
_fIsDECCOLMAllowed(false), // by default, DECCOLM is not allowed.
_fChangedBackground(false),
_fChangedForeground(false),
_fChangedMetaAttrs(false),
_TermOutput()
{
// The top-left corner in VT-speak is 1,1. Our internal array uses 0 indexes, but VT uses 1,1 for top left corner.
_coordSavedCursor.X = 1;
_coordSavedCursor.Y = 1;
_srScrollMargins = { 0 }; // initially, there are no scroll margins.
}

Expand Down Expand Up @@ -420,7 +417,7 @@ bool AdaptDispatch::CursorPosition(_In_ unsigned int const uiLine, _In_ unsigned
// - <none>
// Return Value:
// - True if handled successfully. False otherwise.
bool AdaptDispatch::CursorSavePosition()
bool AdaptDispatch::CursorSaveState()
{
// First retrieve some information about the buffer
CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 };
Expand All @@ -429,6 +426,9 @@ bool AdaptDispatch::CursorSavePosition()
// before the user scrolled the console output
bool fSuccess = !!(_conApi->MoveToBottom() && _conApi->GetConsoleScreenBufferInfoEx(&csbiex));

TextAttribute attributes;
fSuccess = fSuccess && !!(_conApi->PrivateGetTextAttributes(&attributes));

if (fSuccess)
{
// The cursor is given to us by the API as relative to the whole buffer.
Expand All @@ -438,9 +438,12 @@ bool AdaptDispatch::CursorSavePosition()
SMALL_RECT const srViewport = csbiex.srWindow;

// VT is also 1 based, not 0 based, so correct by 1.
_coordSavedCursor.X = coordCursor.X - srViewport.Left + 1;
_coordSavedCursor.Y = coordCursor.Y - srViewport.Top + 1;
_fIsSavedOriginModeRelative = _fIsOriginModeRelative;
auto& savedCursorState = _savedCursorState[_usingAltBuffer];
savedCursorState.Column = coordCursor.X - srViewport.Left + 1;
savedCursorState.Row = coordCursor.Y - srViewport.Top + 1;
savedCursorState.IsOriginModeRelative = _fIsOriginModeRelative;
savedCursorState.Attributes = attributes;
savedCursorState.TermOutput = _TermOutput;
}

return fSuccess;
Expand All @@ -453,17 +456,34 @@ bool AdaptDispatch::CursorSavePosition()
// - <none>
// Return Value:
// - True if handled successfully. False otherwise.
bool AdaptDispatch::CursorRestorePosition()
bool AdaptDispatch::CursorRestoreState()
{
unsigned int const uiRow = _coordSavedCursor.Y;
unsigned int const uiCol = _coordSavedCursor.X;
auto& savedCursorState = _savedCursorState[_usingAltBuffer];

auto uiRow = savedCursorState.Row;
auto uiCol = savedCursorState.Column;

// If the origin mode is relative, and the scrolling region is set (the bottom is non-zero),
// we need to make sure the restored position is clamped within the margins.
if (savedCursorState.IsOriginModeRelative && _srScrollMargins.Bottom != 0)
{
// VT origin is at 1,1 so we need to add 1 to these margins.
uiRow = std::clamp(uiRow, _srScrollMargins.Top + 1u, _srScrollMargins.Bottom + 1u);
}

// The saved coordinates are always absolute, so we need reset the origin mode temporarily.
_fIsOriginModeRelative = false;
bool const fSuccess = _CursorMovePosition(&uiRow, &uiCol);
bool fSuccess = _CursorMovePosition(&uiRow, &uiCol);

// Once the cursor position is restored, we can then restore the actual origin mode.
_fIsOriginModeRelative = _fIsSavedOriginModeRelative;
_fIsOriginModeRelative = savedCursorState.IsOriginModeRelative;

// Restore text attributes.
fSuccess = !!(_conApi->PrivateSetTextAttributes(savedCursorState.Attributes)) && fSuccess;

// Restore designated character set.
_TermOutput = savedCursorState.TermOutput;

return fSuccess;
}

Expand Down Expand Up @@ -1349,7 +1369,16 @@ bool AdaptDispatch::SetWindowTitle(std::wstring_view title)
// - True if handled successfully. False otherwise.
bool AdaptDispatch::UseAlternateScreenBuffer()
{
return !!_conApi->PrivateUseAlternateScreenBuffer();
bool fSuccess = CursorSaveState();
if (fSuccess)
{
fSuccess = !!_conApi->PrivateUseAlternateScreenBuffer();
if (fSuccess)
{
_usingAltBuffer = true;
}
}
return fSuccess;
}

// Routine Description:
Expand All @@ -1361,7 +1390,16 @@ bool AdaptDispatch::UseAlternateScreenBuffer()
// - True if handled successfully. False otherwise.
bool AdaptDispatch::UseMainScreenBuffer()
{
return !!_conApi->PrivateUseMainScreenBuffer();
bool fSuccess = !!_conApi->PrivateUseMainScreenBuffer();
if (fSuccess)
{
_usingAltBuffer = false;
if (fSuccess)
{
fSuccess = CursorRestoreState();
}
}
return fSuccess;
}

//Routine Description:
Expand Down Expand Up @@ -1501,9 +1539,11 @@ bool AdaptDispatch::SoftReset()
}
if (fSuccess)
{
// Save cursor state: Home position; Absolute addressing.
_coordSavedCursor = { 1, 1 };
_fIsSavedOriginModeRelative = false;
// Reset the saved cursor state.
// Note that XTerm only resets the main buffer state, but that
// seems likely to be a bug. Most other terminals reset both.
_savedCursorState[0] = {}; // Main buffer
_savedCursorState[1] = {}; // Alt buffer
}

return fSuccess;
Expand Down
17 changes: 13 additions & 4 deletions src/terminal/adapter/adaptDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ namespace Microsoft::Console::VirtualTerminal
bool CursorHorizontalPositionAbsolute(_In_ unsigned int const uiColumn) override; // CHA
bool VerticalLinePositionAbsolute(_In_ unsigned int const uiLine) override; // VPA
bool CursorPosition(_In_ unsigned int const uiLine, _In_ unsigned int const uiColumn) override; // CUP
bool CursorSavePosition() override; // DECSC
bool CursorRestorePosition() override; // DECRC
bool CursorSaveState() override; // DECSC
bool CursorRestoreState() override; // DECRC
bool CursorVisibility(const bool fIsVisible) override; // DECTCEM
bool EraseInDisplay(const DispatchTypes::EraseType eraseType) override; // ED
bool EraseInLine(const DispatchTypes::EraseType eraseType) override; // EL
Expand Down Expand Up @@ -120,6 +120,14 @@ namespace Microsoft::Console::VirtualTerminal
Up,
Down
};
struct CursorState
{
unsigned int Row = 1;
unsigned int Column = 1;
bool IsOriginModeRelative = false;
TextAttribute Attributes = {};
TerminalOutput TermOutput = {};
};

bool _CursorMovement(const CursorDirection dir, _In_ unsigned int const uiDistance) const;
bool _CursorMovePosition(_In_opt_ const unsigned int* const puiRow, _In_opt_ const unsigned int* const puiCol) const;
Expand Down Expand Up @@ -148,11 +156,12 @@ namespace Microsoft::Console::VirtualTerminal
std::unique_ptr<AdaptDefaults> _pDefaults;
TerminalOutput _TermOutput;

COORD _coordSavedCursor;
CursorState _savedCursorState[2];
bool _usingAltBuffer;

SMALL_RECT _srScrollMargins;

bool _fIsOriginModeRelative;
bool _fIsSavedOriginModeRelative;

bool _fIsSetColumnsEnabled;

Expand Down
3 changes: 3 additions & 0 deletions src/terminal/adapter/conGetSet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Author(s):
#pragma once

#include "..\..\types\inc\IInputEvent.hpp"
#include "..\..\buffer\out\TextAttribute.hpp"
#include "..\..\inc\conattrs.hpp"

#include <deque>
Expand Down Expand Up @@ -54,6 +55,8 @@ namespace Microsoft::Console::VirtualTerminal
virtual BOOL PrivateBoldText(const bool bolded) = 0;
virtual BOOL PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) = 0;
virtual BOOL PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) = 0;
virtual BOOL PrivateGetTextAttributes(TextAttribute* const pAttrs) const = 0;
virtual BOOL PrivateSetTextAttributes(const TextAttribute& attrs) = 0;

virtual BOOL PrivateWriteConsoleInputW(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& events,
_Out_ size_t& eventsWritten) = 0;
Expand Down
Loading