From 1bde1a51169a6bd26e643061bae23b3ff71a7602 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 17 Oct 2019 13:47:37 +0100 Subject: [PATCH 01/12] Add LineFeed method to the ITermDispatch interface that can handle the different linefeed/newline variants required by VT mode. --- src/terminal/adapter/DispatchTypes.hpp | 7 +++++++ src/terminal/adapter/ITermDispatch.hpp | 1 + src/terminal/adapter/adaptDispatch.cpp | 24 ++++++++++++++++++++++++ src/terminal/adapter/adaptDispatch.hpp | 1 + src/terminal/adapter/termDispatch.hpp | 1 + 5 files changed, 34 insertions(+) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 17f26c66d85..df80c5f67a4 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -125,6 +125,13 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes SteadyBar = 6 }; + enum class LineFeedType : unsigned int + { + WithReturn, + WithoutReturn, + DependsOnMode + }; + constexpr short s_sDECCOLMSetColumns = 132; constexpr short s_sDECCOLMResetColumns = 80; diff --git a/src/terminal/adapter/ITermDispatch.hpp b/src/terminal/adapter/ITermDispatch.hpp index 782c6b14d8e..69531119f9a 100644 --- a/src/terminal/adapter/ITermDispatch.hpp +++ b/src/terminal/adapter/ITermDispatch.hpp @@ -54,6 +54,7 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch virtual bool EnableCursorBlinking(const bool enable) = 0; // ATT610 virtual bool SetOriginMode(const bool relativeMode) = 0; // DECOM virtual bool SetTopBottomScrollingMargins(const size_t topMargin, const size_t bottomMargin) = 0; // DECSTBM + virtual bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) = 0; // IND, NEL virtual bool ReverseLineFeed() = 0; // RI virtual bool SetWindowTitle(std::wstring_view title) = 0; // OscWindowTitle virtual bool UseAlternateScreenBuffer() = 0; // ASBSET diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index a886e272d5a..ccf73197348 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -1315,6 +1315,30 @@ bool AdaptDispatch::SetTopBottomScrollingMargins(const size_t topMargin, return _DoSetTopBottomScrollingMargins(topMargin, bottomMargin) && CursorPosition(1, 1); } +// Routine Description: +// - IND/NEL - Performs a line feed, possibly preceded by carriage return. +// Moves the cursor down one line, and possibly also to the leftmost column. +// Arguments: +// - lineFeedType - Specify whether a carriage return should be performed as well. +// Return Value: +// - True if handled successfully. False otherwise. +bool AdaptDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) +{ + switch (lineFeedType) + { + case DispatchTypes::LineFeedType::DependsOnMode: + // Until we support the LNM mode, default to no carriage return. + case DispatchTypes::LineFeedType::WithoutReturn: + Execute(L'\n'); + break; + case DispatchTypes::LineFeedType::WithReturn: + Execute(L'\r'); + Execute(L'\n'); + break; + } + return true; +} + // Routine Description: // - RI - Performs a "Reverse line feed", essentially, the opposite of '\n'. // Moves the cursor up one line, and tries to keep its position in the line diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 9ff6a98708a..de4babaed40 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -69,6 +69,7 @@ namespace Microsoft::Console::VirtualTerminal bool SetOriginMode(const bool relativeMode) noexcept override; // DECOM bool SetTopBottomScrollingMargins(const size_t topMargin, const size_t bottomMargin) override; // DECSTBM + bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) override; // IND, NEL bool ReverseLineFeed() override; // RI bool SetWindowTitle(const std::wstring_view title) override; // OscWindowTitle bool UseAlternateScreenBuffer() override; // ASBSET diff --git a/src/terminal/adapter/termDispatch.hpp b/src/terminal/adapter/termDispatch.hpp index fe889a1b316..9d189ce57bf 100644 --- a/src/terminal/adapter/termDispatch.hpp +++ b/src/terminal/adapter/termDispatch.hpp @@ -48,6 +48,7 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons bool EnableCursorBlinking(const bool /*enable*/) noexcept override { return false; } // ATT610 bool SetOriginMode(const bool /*relativeMode*/) noexcept override { return false; }; // DECOM bool SetTopBottomScrollingMargins(const size_t /*topMargin*/, const size_t /*bottomMargin*/) noexcept override { return false; } // DECSTBM + bool LineFeed(const DispatchTypes::LineFeedType /*lineFeedType*/) override { return false; } // IND, NEL bool ReverseLineFeed() noexcept override { return false; } // RI bool SetWindowTitle(std::wstring_view /*title*/) noexcept override { return false; } // OscWindowTitle bool UseAlternateScreenBuffer() noexcept override { return false; } // ASBSET From 05158e1aa3974bcf8dd36107169e99185530ff71 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 17 Oct 2019 17:21:59 +0100 Subject: [PATCH 02/12] Add support for the NEL escape sequence. --- src/terminal/parser/OutputStateMachineEngine.cpp | 4 ++++ src/terminal/parser/OutputStateMachineEngine.hpp | 1 + src/terminal/parser/telemetry.cpp | 1 + src/terminal/parser/telemetry.hpp | 1 + 4 files changed, 7 insertions(+) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 5e658212649..43158f89437 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -184,6 +184,10 @@ bool OutputStateMachineEngine::ActionEscDispatch(const wchar_t wch, success = _dispatch->SetKeypadMode(false); TermTelemetry::Instance().Log(TermTelemetry::Codes::DECKPNM); break; + case VTActionCodes::NEL_NextLine: + success = _dispatch->LineFeed(DispatchTypes::LineFeedType::WithReturn); + TermTelemetry::Instance().Log(TermTelemetry::Codes::NEL); + break; case VTActionCodes::RI_ReverseLineFeed: success = _dispatch->ReverseLineFeed(); TermTelemetry::Instance().Log(TermTelemetry::Codes::RI); diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp index 0498b2d43d3..6e9f25fa01f 100644 --- a/src/terminal/parser/OutputStateMachineEngine.hpp +++ b/src/terminal/parser/OutputStateMachineEngine.hpp @@ -106,6 +106,7 @@ namespace Microsoft::Console::VirtualTerminal HPA_HorizontalPositionAbsolute = L'`', VPA_VerticalLinePositionAbsolute = L'd', DECSTBM_SetScrollingRegion = L'r', + NEL_NextLine = L'E', // Not a CSI, so doesn't overlap with CNL RI_ReverseLineFeed = L'M', HTS_HorizontalTabSet = L'H', // Not a CSI, so doesn't overlap with CUP CHT_CursorForwardTab = L'I', diff --git a/src/terminal/parser/telemetry.cpp b/src/terminal/parser/telemetry.cpp index 670ced82461..17111fbdd44 100644 --- a/src/terminal/parser/telemetry.cpp +++ b/src/terminal/parser/telemetry.cpp @@ -235,6 +235,7 @@ void TermTelemetry::WriteFinalTraceLog() const TraceLoggingUInt32(_uiTimesUsed[ANSISYSSC], "ANSISYSSC"), TraceLoggingUInt32(_uiTimesUsed[ANSISYSRC], "ANSISYSRC"), TraceLoggingUInt32(_uiTimesUsed[DECSTBM], "DECSTBM"), + TraceLoggingUInt32(_uiTimesUsed[NEL], "NEL"), TraceLoggingUInt32(_uiTimesUsed[RI], "RI"), TraceLoggingUInt32(_uiTimesUsed[OSCWT], "OscWindowTitle"), TraceLoggingUInt32(_uiTimesUsed[HTS], "HTS"), diff --git a/src/terminal/parser/telemetry.hpp b/src/terminal/parser/telemetry.hpp index 91343f81f15..3f04a855158 100644 --- a/src/terminal/parser/telemetry.hpp +++ b/src/terminal/parser/telemetry.hpp @@ -62,6 +62,7 @@ namespace Microsoft::Console::VirtualTerminal IL, DL, DECSTBM, + NEL, RI, OSCWT, HTS, From 829c59dbaba08469ca389f75da8bb47ca8826b2b Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 17 Oct 2019 17:37:58 +0100 Subject: [PATCH 03/12] Add support for LF, FF, and VT control characters. --- .../parser/OutputStateMachineEngine.cpp | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 43158f89437..d68a91df1af 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -39,15 +39,24 @@ ITermDispatch& OutputStateMachineEngine::Dispatch() noexcept // - true iff we successfully dispatched the sequence. bool OutputStateMachineEngine::ActionExecute(const wchar_t wch) { - // microsoft/terminal#1825 - VT applications expect to be able to write NUL - // and have _nothing_ happen. Filter the NULs here, so they don't fill the - // buffer with empty spaces. - if (wch == AsciiChars::NUL) + switch (wch) { - return true; + case AsciiChars::NUL: + // microsoft/terminal#1825 - VT applications expect to be able to write NUL + // and have _nothing_ happen. Filter the NULs here, so they don't fill the + // buffer with empty spaces. + break; + case AsciiChars::LF: + case AsciiChars::FF: + case AsciiChars::VT: + // LF, FF, and VT are identical in function. + _dispatch->LineFeed(DispatchTypes::LineFeedType::DependsOnMode); + break; + default: + _dispatch->Execute(wch); + break; } - _dispatch->Execute(wch); _ClearLastChar(); if (wch == AsciiChars::BEL) From 67152affec3e92d47d591d61381f81639594185a Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 17 Oct 2019 18:38:33 +0100 Subject: [PATCH 04/12] Add support for the IND escape sequence. --- src/terminal/parser/OutputStateMachineEngine.cpp | 4 ++++ src/terminal/parser/OutputStateMachineEngine.hpp | 1 + src/terminal/parser/telemetry.cpp | 1 + src/terminal/parser/telemetry.hpp | 1 + 4 files changed, 7 insertions(+) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index d68a91df1af..6118b1625f2 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -197,6 +197,10 @@ bool OutputStateMachineEngine::ActionEscDispatch(const wchar_t wch, success = _dispatch->LineFeed(DispatchTypes::LineFeedType::WithReturn); TermTelemetry::Instance().Log(TermTelemetry::Codes::NEL); break; + case VTActionCodes::IND_Index: + success = _dispatch->LineFeed(DispatchTypes::LineFeedType::WithoutReturn); + TermTelemetry::Instance().Log(TermTelemetry::Codes::IND); + break; case VTActionCodes::RI_ReverseLineFeed: success = _dispatch->ReverseLineFeed(); TermTelemetry::Instance().Log(TermTelemetry::Codes::RI); diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp index 6e9f25fa01f..0c6b5c02ff4 100644 --- a/src/terminal/parser/OutputStateMachineEngine.hpp +++ b/src/terminal/parser/OutputStateMachineEngine.hpp @@ -107,6 +107,7 @@ namespace Microsoft::Console::VirtualTerminal VPA_VerticalLinePositionAbsolute = L'd', DECSTBM_SetScrollingRegion = L'r', NEL_NextLine = L'E', // Not a CSI, so doesn't overlap with CNL + IND_Index = L'D', // Not a CSI, so doesn't overlap with CUB RI_ReverseLineFeed = L'M', HTS_HorizontalTabSet = L'H', // Not a CSI, so doesn't overlap with CUP CHT_CursorForwardTab = L'I', diff --git a/src/terminal/parser/telemetry.cpp b/src/terminal/parser/telemetry.cpp index 17111fbdd44..2343c38c64c 100644 --- a/src/terminal/parser/telemetry.cpp +++ b/src/terminal/parser/telemetry.cpp @@ -236,6 +236,7 @@ void TermTelemetry::WriteFinalTraceLog() const TraceLoggingUInt32(_uiTimesUsed[ANSISYSRC], "ANSISYSRC"), TraceLoggingUInt32(_uiTimesUsed[DECSTBM], "DECSTBM"), TraceLoggingUInt32(_uiTimesUsed[NEL], "NEL"), + TraceLoggingUInt32(_uiTimesUsed[IND], "IND"), TraceLoggingUInt32(_uiTimesUsed[RI], "RI"), TraceLoggingUInt32(_uiTimesUsed[OSCWT], "OscWindowTitle"), TraceLoggingUInt32(_uiTimesUsed[HTS], "HTS"), diff --git a/src/terminal/parser/telemetry.hpp b/src/terminal/parser/telemetry.hpp index 3f04a855158..d03441e97f5 100644 --- a/src/terminal/parser/telemetry.hpp +++ b/src/terminal/parser/telemetry.hpp @@ -63,6 +63,7 @@ namespace Microsoft::Console::VirtualTerminal DL, DECSTBM, NEL, + IND, RI, OSCWT, HTS, From 06d6f83237f940fb56c289617c51be4cce832cfd Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 17 Oct 2019 20:18:39 +0100 Subject: [PATCH 05/12] Add output engine tests for the various linefeed operations. --- .../parser/ut_parser/OutputEngineTest.cpp | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 79426542806..85a569afb10 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -677,6 +677,8 @@ class StatefulDispatch final : public TermDispatch _cursorKeysMode{ false }, _cursorBlinking{ true }, _isOriginModeRelative{ false }, + _lineFeed{ false }, + _lineFeedType{ (DispatchTypes::LineFeedType)-1 }, _isDECCOLMAllowed{ false }, _windowWidth{ 80 }, _options{ s_cMaxOptions, static_cast(s_uiGraphicsCleared) } // fill with cleared option @@ -904,6 +906,13 @@ class StatefulDispatch final : public TermDispatch return true; } + bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) override + { + _lineFeed = true; + _lineFeedType = lineFeedType; + return true; + } + bool EnableDECCOLMSupport(const bool fEnabled) noexcept override { _isDECCOLMAllowed = fEnabled; @@ -950,6 +959,8 @@ class StatefulDispatch final : public TermDispatch bool _cursorKeysMode; bool _cursorBlinking; bool _isOriginModeRelative; + bool _lineFeed; + DispatchTypes::LineFeedType _lineFeedType; bool _isDECCOLMAllowed; size_t _windowWidth; @@ -1783,4 +1794,54 @@ class StateMachineExternalTest final pDispatch->ClearState(); } + + TEST_METHOD(TestLineFeed) + { + auto dispatch = std::make_unique(); + auto pDispatch = dispatch.get(); + auto engine = std::make_unique(std::move(dispatch)); + StateMachine mach(std::move(engine)); + + Log::Comment(L"IND (Index) escape sequence"); + mach.ProcessCharacter(AsciiChars::ESC); + mach.ProcessCharacter(L'D'); + + VERIFY_IS_TRUE(pDispatch->_lineFeed); + VERIFY_ARE_EQUAL(DispatchTypes::LineFeedType::WithoutReturn, pDispatch->_lineFeedType); + + pDispatch->ClearState(); + + Log::Comment(L"NEL (Next Line) escape sequence"); + mach.ProcessCharacter(AsciiChars::ESC); + mach.ProcessCharacter(L'E'); + + VERIFY_IS_TRUE(pDispatch->_lineFeed); + VERIFY_ARE_EQUAL(DispatchTypes::LineFeedType::WithReturn, pDispatch->_lineFeedType); + + pDispatch->ClearState(); + + Log::Comment(L"LF (Line Feed) control code"); + mach.ProcessCharacter(AsciiChars::LF); + + VERIFY_IS_TRUE(pDispatch->_lineFeed); + VERIFY_ARE_EQUAL(DispatchTypes::LineFeedType::DependsOnMode, pDispatch->_lineFeedType); + + pDispatch->ClearState(); + + Log::Comment(L"FF (Form Feed) control code"); + mach.ProcessCharacter(AsciiChars::FF); + + VERIFY_IS_TRUE(pDispatch->_lineFeed); + VERIFY_ARE_EQUAL(DispatchTypes::LineFeedType::DependsOnMode, pDispatch->_lineFeedType); + + pDispatch->ClearState(); + + Log::Comment(L"VT (Vertical Tab) control code"); + mach.ProcessCharacter(AsciiChars::VT); + + VERIFY_IS_TRUE(pDispatch->_lineFeed); + VERIFY_ARE_EQUAL(DispatchTypes::LineFeedType::DependsOnMode, pDispatch->_lineFeedType); + + pDispatch->ClearState(); + } }; From ad36c89460c584e21248d91a46c80d60a0ce7ee4 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 20 Oct 2019 00:21:51 +0100 Subject: [PATCH 06/12] Add a private API for executing linefeeds without having to go through the WriteCharsLegacy function. --- src/host/getset.cpp | 27 +++++++++++++++++++ src/host/getset.h | 1 + src/host/outputStream.cpp | 13 +++++++++ src/host/outputStream.hpp | 1 + src/terminal/adapter/adaptDispatch.cpp | 10 +++---- src/terminal/adapter/conGetSet.hpp | 1 + .../adapter/ut_adapter/adapterTest.cpp | 7 +++++ 7 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index ad5af0e9a69..1e3bdf10351 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1347,6 +1347,33 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool return Status; } +// Routine Description: +// - A private API call for performing a line feed, possibly preceded by carriage return. +// Moves the cursor down one line, and possibly also to the leftmost column. +// Parameters: +// - screenInfo - A pointer to the screen buffer that should perform the line feed. +// - withReturn - Set to true if a carriage return should be performed as well. +// Return value: +// - STATUS_SUCCESS if handled successfully. Otherwise, an appropriate status code indicating the error. +[[nodiscard]] NTSTATUS DoSrvPrivateLineFeed(SCREEN_INFORMATION& screenInfo, const bool withReturn) +{ + auto& textBuffer = screenInfo.GetTextBuffer(); + auto cursorPosition = textBuffer.GetCursor().GetPosition(); + + textBuffer.GetCursor().SetIsOn(true); + + // Since we are explicitly moving down a row, clear the wrap status on the row we're leaving + textBuffer.GetRowByOffset(cursorPosition.Y).GetCharRow().SetWrapForced(false); + + cursorPosition.Y += 1; + if (withReturn) + { + cursorPosition.X = 0; + } + + return AdjustCursorPosition(screenInfo, cursorPosition, FALSE, nullptr); +} + // Routine Description: // - A private API call for performing a "Reverse line feed", essentially, the opposite of '\n'. // Moves the cursor up one line, and tries to keep its position in the line diff --git a/src/host/getset.h b/src/host/getset.h index 0199a7660f0..47e542156af 100644 --- a/src/host/getset.h +++ b/src/host/getset.h @@ -33,6 +33,7 @@ void DoSrvPrivateShowCursor(SCREEN_INFORMATION& screenInfo, const bool show) noe void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool fEnable); [[nodiscard]] NTSTATUS DoSrvPrivateSetScrollingRegion(SCREEN_INFORMATION& screenInfo, const SMALL_RECT& scrollMargins); +[[nodiscard]] NTSTATUS DoSrvPrivateLineFeed(SCREEN_INFORMATION& screenInfo, const bool withReturn); [[nodiscard]] NTSTATUS DoSrvPrivateReverseLineFeed(SCREEN_INFORMATION& screenInfo); [[nodiscard]] HRESULT DoSrvMoveCursorVertically(SCREEN_INFORMATION& screenInfo, const short lines); diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index ff713ae4156..9287a5e7284 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -386,6 +386,19 @@ bool ConhostInternalGetSet::PrivateSetScrollingRegion(const SMALL_RECT& scrollMa return NT_SUCCESS(DoSrvPrivateSetScrollingRegion(_io.GetActiveOutputBuffer(), scrollMargins)); } +// Routine Description: +// - Connects the PrivateLineFeed call directly into our Driver Message servicing call inside Conhost.exe +// PrivateLineFeed is an internal-only "API" call that the vt commands can execute, +// but it is not represented as a function call on our public API surface. +// Arguments: +// - withReturn - Set to true if a carriage return should be performed as well. +// Return Value: +// - true if successful (see DoSrvPrivateLineFeed). false otherwise. +bool ConhostInternalGetSet::PrivateLineFeed(const bool withReturn) +{ + return NT_SUCCESS(DoSrvPrivateLineFeed(_io.GetActiveOutputBuffer(), withReturn)); +} + // Routine Description: // - Connects the PrivateReverseLineFeed call directly into our Driver Message servicing call inside Conhost.exe // PrivateReverseLineFeed is an internal-only "API" call that the vt commands can execute, diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index f7f6d720f43..3159326ad01 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -98,6 +98,7 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) override; + bool PrivateLineFeed(const bool withReturn) override; bool PrivateReverseLineFeed() override; bool MoveCursorVertically(const ptrdiff_t lines) override; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index ccf73197348..2bcc5efa140 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -1329,14 +1329,12 @@ bool AdaptDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) case DispatchTypes::LineFeedType::DependsOnMode: // Until we support the LNM mode, default to no carriage return. case DispatchTypes::LineFeedType::WithoutReturn: - Execute(L'\n'); - break; + return _pConApi->PrivateLineFeed(false); case DispatchTypes::LineFeedType::WithReturn: - Execute(L'\r'); - Execute(L'\n'); - break; + return _pConApi->PrivateLineFeed(true); + default: + return false; } - return true; } // Routine Description: diff --git a/src/terminal/adapter/conGetSet.hpp b/src/terminal/adapter/conGetSet.hpp index 6b6f9e0b8e6..a8d33788812 100644 --- a/src/terminal/adapter/conGetSet.hpp +++ b/src/terminal/adapter/conGetSet.hpp @@ -61,6 +61,7 @@ namespace Microsoft::Console::VirtualTerminal virtual bool PrivateAllowCursorBlinking(const bool enable) = 0; virtual bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) = 0; + virtual bool PrivateLineFeed(const bool withReturn) = 0; virtual bool PrivateReverseLineFeed() = 0; virtual bool SetConsoleTitleW(const std::wstring_view title) = 0; virtual bool PrivateUseAlternateScreenBuffer() = 0; diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 99d8600729b..4324e8b4997 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -369,6 +369,13 @@ class TestGetSet final : public ConGetSet return _privateSetScrollingRegionResult; } + bool PrivateLineFeed(const bool /*withReturn*/) override + { + Log::Comment(L"PrivateLineFeed MOCK called..."); + // We made it through the adapter, woo! Return true. + return true; + } + bool PrivateReverseLineFeed() override { Log::Comment(L"PrivateReverseLineFeed MOCK called..."); From ef087de39d8e419931d38b8927ec967867ae63b4 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 20 Oct 2019 14:59:33 +0100 Subject: [PATCH 07/12] Use the AutoReturnOnNewline setting as a proxy for the Line Feed/New Line mode. --- src/host/outputStream.cpp | 12 ++++++++++++ src/host/outputStream.hpp | 1 + src/terminal/adapter/adaptDispatch.cpp | 2 +- src/terminal/adapter/conGetSet.hpp | 1 + src/terminal/adapter/ut_adapter/adapterTest.cpp | 7 +++++++ 5 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 9287a5e7284..8c71e7c6bf5 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -386,6 +386,18 @@ bool ConhostInternalGetSet::PrivateSetScrollingRegion(const SMALL_RECT& scrollMa return NT_SUCCESS(DoSrvPrivateSetScrollingRegion(_io.GetActiveOutputBuffer(), scrollMargins)); } +// Method Description: +// - Retrieves the current Line Feed/New Line (LNM) mode. +// Arguments: +// - None +// Return Value: +// - true if a line feed also produces a carriage return. false otherwise. +bool ConhostInternalGetSet::PrivateGetLineFeedMode() const +{ + const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + return gci.IsReturnOnNewlineAutomatic(); +} + // Routine Description: // - Connects the PrivateLineFeed call directly into our Driver Message servicing call inside Conhost.exe // PrivateLineFeed is an internal-only "API" call that the vt commands can execute, diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index 3159326ad01..0efc0923a79 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -98,6 +98,7 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) override; + bool PrivateGetLineFeedMode() const override; bool PrivateLineFeed(const bool withReturn) override; bool PrivateReverseLineFeed() override; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 2bcc5efa140..2ebb4dd7e91 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -1327,7 +1327,7 @@ bool AdaptDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) switch (lineFeedType) { case DispatchTypes::LineFeedType::DependsOnMode: - // Until we support the LNM mode, default to no carriage return. + return _pConApi->PrivateLineFeed(_pConApi->PrivateGetLineFeedMode()); case DispatchTypes::LineFeedType::WithoutReturn: return _pConApi->PrivateLineFeed(false); case DispatchTypes::LineFeedType::WithReturn: diff --git a/src/terminal/adapter/conGetSet.hpp b/src/terminal/adapter/conGetSet.hpp index a8d33788812..671519d572c 100644 --- a/src/terminal/adapter/conGetSet.hpp +++ b/src/terminal/adapter/conGetSet.hpp @@ -61,6 +61,7 @@ namespace Microsoft::Console::VirtualTerminal virtual bool PrivateAllowCursorBlinking(const bool enable) = 0; virtual bool PrivateSetScrollingRegion(const SMALL_RECT& scrollMargins) = 0; + virtual bool PrivateGetLineFeedMode() const = 0; virtual bool PrivateLineFeed(const bool withReturn) = 0; virtual bool PrivateReverseLineFeed() = 0; virtual bool SetConsoleTitleW(const std::wstring_view title) = 0; diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 4324e8b4997..223a7f8ae24 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -369,6 +369,13 @@ class TestGetSet final : public ConGetSet return _privateSetScrollingRegionResult; } + bool PrivateGetLineFeedMode() const override + { + Log::Comment(L"PrivateGetLineFeedMode MOCK called..."); + // Not currently used. Return true for now. + return true; + } + bool PrivateLineFeed(const bool /*withReturn*/) override { Log::Comment(L"PrivateLineFeed MOCK called..."); From 99c87415d7d5a148259279aee9ec9f7d23406587 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 20 Oct 2019 16:08:10 +0100 Subject: [PATCH 08/12] Add a minimal implementation of the LineFeed method in the TerminalDispatch class. --- .../TerminalCore/TerminalDispatch.cpp | 19 +++++++++++++++++++ .../TerminalCore/TerminalDispatch.hpp | 2 ++ 2 files changed, 21 insertions(+) diff --git a/src/cascadia/TerminalCore/TerminalDispatch.cpp b/src/cascadia/TerminalCore/TerminalDispatch.cpp index 910c65dea56..0429dc4d64d 100644 --- a/src/cascadia/TerminalCore/TerminalDispatch.cpp +++ b/src/cascadia/TerminalCore/TerminalDispatch.cpp @@ -74,6 +74,25 @@ try } CATCH_LOG_RETURN_FALSE() +bool TerminalDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) +{ + switch (lineFeedType) + { + case DispatchTypes::LineFeedType::DependsOnMode: + // There is currently no need for mode-specific line feeds in the Terminal, + // so for now we just treat them as a line feed without carriage return. + case DispatchTypes::LineFeedType::WithoutReturn: + Execute(L'\n'); + return true; + case DispatchTypes::LineFeedType::WithReturn: + Execute(L'\r'); + Execute(L'\n'); + return true; + default: + return false; + } +} + bool TerminalDispatch::EraseCharacters(const size_t numChars) noexcept try { diff --git a/src/cascadia/TerminalCore/TerminalDispatch.hpp b/src/cascadia/TerminalCore/TerminalDispatch.hpp index a617ec2e266..2b3c3af9992 100644 --- a/src/cascadia/TerminalCore/TerminalDispatch.hpp +++ b/src/cascadia/TerminalCore/TerminalDispatch.hpp @@ -22,6 +22,8 @@ class TerminalDispatch : public Microsoft::Console::VirtualTerminal::TermDispatc bool CursorBackward(const size_t distance) noexcept override; bool CursorUp(const size_t distance) noexcept override; + bool LineFeed(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::LineFeedType lineFeedType) override; + bool EraseCharacters(const size_t numChars) noexcept override; bool SetWindowTitle(std::wstring_view title) noexcept override; From 1414198b078f0b7f8b83ab8581abed58850d0227 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 20 Oct 2019 16:41:34 +0100 Subject: [PATCH 09/12] Add adapter tests for the various linefeed types. --- .../adapter/ut_adapter/adapterTest.cpp | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 223a7f8ae24..d30c665d14f 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -372,15 +372,19 @@ class TestGetSet final : public ConGetSet bool PrivateGetLineFeedMode() const override { Log::Comment(L"PrivateGetLineFeedMode MOCK called..."); - // Not currently used. Return true for now. - return true; + return _privateGetLineFeedModeResult; } - bool PrivateLineFeed(const bool /*withReturn*/) override + bool PrivateLineFeed(const bool withReturn) override { Log::Comment(L"PrivateLineFeed MOCK called..."); - // We made it through the adapter, woo! Return true. - return true; + + if (_privateLineFeedResult) + { + VERIFY_ARE_EQUAL(_expectedLineFeedWithReturn, withReturn); + } + + return _privateLineFeedResult; } bool PrivateReverseLineFeed() override @@ -907,6 +911,9 @@ class TestGetSet final : public ConGetSet bool _privateAllowCursorBlinkingResult = false; bool _enable = false; // for cursor blinking bool _privateSetScrollingRegionResult = false; + bool _privateGetLineFeedModeResult = false; + bool _privateLineFeedResult = false; + bool _expectedLineFeedWithReturn = false; bool _privateReverseLineFeedResult = false; bool _setConsoleTitleWResult = false; @@ -2123,6 +2130,32 @@ class AdapterTest VERIFY_IS_FALSE(_pDispatch.get()->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom)); } + TEST_METHOD(LineFeedTest) + { + Log::Comment(L"Starting test..."); + + // All test cases need the LineFeed call to succeed. + _testGetSet->_privateLineFeedResult = TRUE; + + Log::Comment(L"Test 1: Line feed without carriage return."); + _testGetSet->_expectedLineFeedWithReturn = false; + VERIFY_IS_TRUE(_pDispatch.get()->LineFeed(DispatchTypes::LineFeedType::WithoutReturn)); + + Log::Comment(L"Test 2: Line feed with carriage return."); + _testGetSet->_expectedLineFeedWithReturn = true; + VERIFY_IS_TRUE(_pDispatch.get()->LineFeed(DispatchTypes::LineFeedType::WithReturn)); + + Log::Comment(L"Test 3: Line feed depends on mode, and mode reset."); + _testGetSet->_privateGetLineFeedModeResult = false; + _testGetSet->_expectedLineFeedWithReturn = false; + VERIFY_IS_TRUE(_pDispatch.get()->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); + + Log::Comment(L"Test 4: Line feed depends on mode, and mode set."); + _testGetSet->_privateGetLineFeedModeResult = true; + _testGetSet->_expectedLineFeedWithReturn = true; + VERIFY_IS_TRUE(_pDispatch.get()->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); + } + TEST_METHOD(TabSetClearTests) { Log::Comment(L"Starting test..."); From b08fd85962446e92349afa11f2f6403d32bebf76 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Mon, 21 Oct 2019 10:32:10 +0100 Subject: [PATCH 10/12] Add screen buffer tests for the IND and NEL escape sequences. --- src/host/ut_host/ScreenBufferTests.cpp | 89 ++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index fa6fd102900..02562242a84 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -178,6 +178,8 @@ class ScreenBufferTests TEST_METHOD(DeleteLinesInMargins); TEST_METHOD(ReverseLineFeedInMargins); + TEST_METHOD(LineFeedEscapeSequences); + TEST_METHOD(ScrollLines256Colors); TEST_METHOD(SetOriginMode); @@ -4264,6 +4266,8 @@ void ScreenBufferTests::ReverseLineFeedInMargins() _CommonScrollingSetup(); // Set the top scroll margin to the top of the screen stateMachine.ProcessString(L"\x1b[1;5r"); + // Make sure we clear the margins on exit so they can't break other tests. + auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); }); // Move to column 5 of line 1, the top of the screen stateMachine.ProcessString(L"\x1b[1;5H"); // Execute a reverse line feed (RI) @@ -4292,6 +4296,91 @@ void ScreenBufferTests::ReverseLineFeedInMargins() } } +void ScreenBufferTests::LineFeedEscapeSequences() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:withReturn", L"{true, false}") + END_TEST_METHOD_PROPERTIES() + + bool withReturn; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"withReturn", withReturn)); + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si.GetTextBuffer().GetCursor(); + + std::wstring escapeSequence; + if (withReturn) + { + Log::Comment(L"Testing line feed with carriage return (NEL)."); + escapeSequence = L"\033E"; + } + else + { + Log::Comment(L"Testing line feed without carriage return (IND)."); + escapeSequence = L"\033D"; + } + + // Set the viewport to a reasonable size. + const auto view = Viewport::FromDimensions({ 0, 0 }, { 80, 25 }); + si.SetViewport(view, true); + + // We'll place the cursor in the center of the line. + // If we are performing a line feed with carriage return, + // the cursor should move to the leftmost column. + const short initialX = view.Width() / 2; + const short expectedX = withReturn ? 0 : initialX; + + { + Log::Comment(L"Starting at the top of viewport"); + const short initialY = 0; + const short expectedY = initialY + 1; + const short expectedViewportTop = si.GetViewport().Top(); + cursor.SetPosition(COORD{ initialX, initialY }); + stateMachine.ProcessString(escapeSequence); + + VERIFY_ARE_EQUAL(expectedX, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(expectedY, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(expectedViewportTop, si.GetViewport().Top()); + } + + { + Log::Comment(L"Starting at the bottom of viewport"); + const short initialY = si.GetViewport().BottomInclusive(); + const short expectedY = initialY + 1; + const short expectedViewportTop = si.GetViewport().Top() + 1; + cursor.SetPosition(COORD{ initialX, initialY }); + stateMachine.ProcessString(escapeSequence); + + VERIFY_ARE_EQUAL(expectedX, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(expectedY, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(expectedViewportTop, si.GetViewport().Top()); + } + + { + Log::Comment(L"Starting at the bottom of the scroll margins"); + // Set the margins to rows 5 to 10. + stateMachine.ProcessString(L"\x1b[5;10r"); + // Make sure we clear the margins on exit so they can't break other tests. + auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); }); + + const short initialY = si.GetViewport().Top() + 9; + const short expectedY = initialY; + const short expectedViewportTop = si.GetViewport().Top(); + _FillLine(initialY, L'Q', {}); + cursor.SetPosition(COORD{ initialX, initialY }); + stateMachine.ProcessString(escapeSequence); + + VERIFY_ARE_EQUAL(expectedX, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(expectedY, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(expectedViewportTop, si.GetViewport().Top()); + // Verify the line of Qs has been scrolled up. + VERIFY_IS_TRUE(_ValidateLineContains(initialY - 1, L'Q', {})); + VERIFY_IS_TRUE(_ValidateLineContains(initialY, L' ', si.GetAttributes())); + } +} + void ScreenBufferTests::ScrollLines256Colors() { BEGIN_TEST_METHOD_PROPERTIES() From db477bf3b4456c253747e63cf9664d56ce0d3ff5 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sat, 4 Jan 2020 22:45:47 +0000 Subject: [PATCH 11/12] Make the TerminalDispatch::LineFeed method noexcept, to match the new audit style. --- src/cascadia/TerminalCore/TerminalDispatch.cpp | 4 +++- src/cascadia/TerminalCore/TerminalDispatch.hpp | 2 +- src/terminal/adapter/termDispatch.hpp | 2 +- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalDispatch.cpp b/src/cascadia/TerminalCore/TerminalDispatch.cpp index 0429dc4d64d..f9b9d61b7d5 100644 --- a/src/cascadia/TerminalCore/TerminalDispatch.cpp +++ b/src/cascadia/TerminalCore/TerminalDispatch.cpp @@ -74,7 +74,8 @@ try } CATCH_LOG_RETURN_FALSE() -bool TerminalDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) +bool TerminalDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) noexcept +try { switch (lineFeedType) { @@ -92,6 +93,7 @@ bool TerminalDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) return false; } } +CATCH_LOG_RETURN_FALSE() bool TerminalDispatch::EraseCharacters(const size_t numChars) noexcept try diff --git a/src/cascadia/TerminalCore/TerminalDispatch.hpp b/src/cascadia/TerminalCore/TerminalDispatch.hpp index 2b3c3af9992..d0c59a2e900 100644 --- a/src/cascadia/TerminalCore/TerminalDispatch.hpp +++ b/src/cascadia/TerminalCore/TerminalDispatch.hpp @@ -22,7 +22,7 @@ class TerminalDispatch : public Microsoft::Console::VirtualTerminal::TermDispatc bool CursorBackward(const size_t distance) noexcept override; bool CursorUp(const size_t distance) noexcept override; - bool LineFeed(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::LineFeedType lineFeedType) override; + bool LineFeed(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::LineFeedType lineFeedType) noexcept override; bool EraseCharacters(const size_t numChars) noexcept override; bool SetWindowTitle(std::wstring_view title) noexcept override; diff --git a/src/terminal/adapter/termDispatch.hpp b/src/terminal/adapter/termDispatch.hpp index 9d189ce57bf..c59cf2aa397 100644 --- a/src/terminal/adapter/termDispatch.hpp +++ b/src/terminal/adapter/termDispatch.hpp @@ -48,7 +48,7 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons bool EnableCursorBlinking(const bool /*enable*/) noexcept override { return false; } // ATT610 bool SetOriginMode(const bool /*relativeMode*/) noexcept override { return false; }; // DECOM bool SetTopBottomScrollingMargins(const size_t /*topMargin*/, const size_t /*bottomMargin*/) noexcept override { return false; } // DECSTBM - bool LineFeed(const DispatchTypes::LineFeedType /*lineFeedType*/) override { return false; } // IND, NEL + bool LineFeed(const DispatchTypes::LineFeedType /*lineFeedType*/) noexcept override { return false; } // IND, NEL bool ReverseLineFeed() noexcept override { return false; } // RI bool SetWindowTitle(std::wstring_view /*title*/) noexcept override { return false; } // OscWindowTitle bool UseAlternateScreenBuffer() noexcept override { return false; } // ASBSET diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 85a569afb10..a34b843addd 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -906,7 +906,7 @@ class StatefulDispatch final : public TermDispatch return true; } - bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) override + bool LineFeed(const DispatchTypes::LineFeedType lineFeedType) noexcept override { _lineFeed = true; _lineFeedType = lineFeedType; From df0861f95ce2370728ec7e8599ac1ab2e6eb440a Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 5 Jan 2020 00:06:36 +0000 Subject: [PATCH 12/12] Add comment explaining why the cursor needs to be turned on for a line feed. --- src/host/getset.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 1e3bdf10351..bbc477769ec 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1360,6 +1360,8 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool auto& textBuffer = screenInfo.GetTextBuffer(); auto cursorPosition = textBuffer.GetCursor().GetPosition(); + // We turn the cursor on before an operation that might scroll the viewport, otherwise + // that can result in an old copy of the cursor being left behind on the screen. textBuffer.GetCursor().SetIsOn(true); // Since we are explicitly moving down a row, clear the wrap status on the row we're leaving