Merged PR 3813180: Migrate selected GitHub changes up to 6b728cd

- Remember last-used string in the Find dialog in conhost (GH-2845)

  (cherry picked from commit bfb1484)

- Bugfix: TextBuffer Line Wrapping from VTRenderer (GH-2797)

  Change VT renderer to do erase line instead of a ton of erase chars

  (cherry picked from commit 8afc5b2)

- Add some retry support to Renderer::PaintFrame (GH-2830)

  If _PaintFrameForEngine returns E_PENDING, we'll give it another two
  tries to get itself straight. If it continues to fail, we'll take down
  the application.

  We observed that the DX renderer was failing to present the swap chain
  and failfast'ing when it did so; however, there are some errors from
  which DXGI guidance suggests we try to recover. We'll now return
  E_PENDING (and destroy our device resources) when we hit those errors.

  Fixes GH-2265.

  (cherry picked from commit 277acc3)

- Enable VT processing by default for ConPTY (GH-2824)

  This change enables VT processing by default for _all_ conpty clients. See
  GH-1965 for a discussion on why we believe this is a righteous change.

  Also mentioned in the issue was the idea of only checking the
  `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would
  be a more difficult change, looks like all we'd need is a simple
  `reg.LoadGlobalsFromRegistry();` call instead of this change.

  **Validation Steps Performed**
  Manually launched a scratch app in both the terminal and the console. The
  console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the `
  ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default
  in the Terminal.

  Closes GH-1965

  (cherry picked from commit 1c412d4)

- Remove unwanted DECSTBM clipping (GH-2764)

  The `DECSTBM` margins are meant to define the range of lines within which
  certain vertical scrolling operations take place. However, we were applying
  these margin restrictions in the `ScrollRegion` function, which is also used in
  a number of places that shouldn't be affected by `DECSTBM`.

  This includes the `ICH` and `DCH` escape sequences (which are only affected by
  the horizontal margins, which we don't yet support), the
  `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be
  affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback
  extension (which isn't really scrolling as such, but uses the `ScrollRegion`
  function to manipulate the scrollback buffer).

  This commit moves the margin clipping out of the `ScrollRegion` function, so it
  can be applied exclusively in the places that need it.

  While testing, I also noticed that one of the `ScrollRegion` calls in the
  `AdjustCursorPosition` function was not setting the horizontal range correctly
  - the scrolling should always affect the full buffer width rather than just the
    viewport width - so ...

Related work items: #23507749, #23563809, #23563819, #23563837, #23563841, #23563844

Author: DHowett

src/cascadia/TerminalCore/lib/terminalcore-lib.vcxproj

@@ -13,7 +13,7 @@
     <ProjectName>TerminalCore</ProjectName>
     <TargetName>TerminalCore</TargetName>
     <WindowsTargetPlatformMinVersion>10.0.17763.0</WindowsTargetPlatformMinVersion>
-    <WindowsTargetPlatformVersion>10.0.17763.0</WindowsTargetPlatformVersion>
+    <WindowsTargetPlatformVersion>10.0.18362.0</WindowsTargetPlatformVersion>
     <RootNamespace>Microsoft.Terminal.Core</RootNamespace>
   </PropertyGroup>
   <!-- ============================ References ============================  -->

src/host/_stream.cpp

@@ -140,15 +140,11 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
         const COORD newPostMarginsOrigin = { 0, moveToYPosition };
         const COORD newViewOrigin = { 0, newViewTop };
 
-        // Unset the margins to scroll the content below the margins,
-        //      then restore them after.
-        screenInfo.SetScrollMargins(Viewport::FromInclusive({ 0 }));
         try
         {
             ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, bufferAttributes);
         }
         CATCH_LOG();
-        screenInfo.SetScrollMargins(relativeMargins);
 
         // Move the viewport down
         auto hr = screenInfo.SetViewportOrigin(true, newViewOrigin, true);
@@ -186,39 +182,19 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
         SMALL_RECT scrollRect = { 0 };
         scrollRect.Top = srMargins.Top;
         scrollRect.Bottom = srMargins.Bottom;
-        scrollRect.Left = screenInfo.GetViewport().Left(); // NOTE: Left/Right Scroll margins don't do anything currently.
-        scrollRect.Right = screenInfo.GetViewport().RightInclusive();
+        scrollRect.Left = 0; // NOTE: Left/Right Scroll margins don't do anything currently.
+        scrollRect.Right = bufferSize.X - 1; // -1, otherwise this would be an exclusive rect.
 
         COORD dest;
         dest.X = scrollRect.Left;
         dest.Y = scrollRect.Top - diff;
 
-        SMALL_RECT clipRect = scrollRect;
-        // Typically ScrollRegion() clips by the scroll margins. However, if
-        //      we're scrolling down at the top of the viewport, we'll need to
-        //      not clip at the margins, instead move the contents of the margins
-        //      up above the viewport. So we'll clear out the current margins, and
-        //      set them to the viewport+(#diff rows above the viewport).
-        if (scrollDownAtTop)
-        {
-            clipRect.Top -= diff;
-            auto fakeMargins = srMargins;
-            fakeMargins.Top -= diff;
-            auto fakeRelative = viewport.ConvertToOrigin(Viewport::FromInclusive(fakeMargins));
-            screenInfo.SetScrollMargins(fakeRelative);
-        }
-
         try
         {
-            ScrollRegion(screenInfo, scrollRect, clipRect, dest, UNICODE_SPACE, bufferAttributes);
+            ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, bufferAttributes);
         }
         CATCH_LOG();
 
-        if (scrollDownAtTop)
-        {
-            // Undo the fake margins we set above
-            screenInfo.SetScrollMargins(relativeMargins);
-        }
         coordCursor.Y -= diff;
     }
 

src/host/getset.cpp

@@ -1368,6 +1368,11 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
             srScroll.Right = SHORT_MAX;
             srScroll.Top = viewport.Top;
             srScroll.Bottom = viewport.Bottom;
+            // Clip to the DECSTBM margin boundary
+            if (screenInfo.AreMarginsSet())
+            {
+                srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive();
+            }
             // Paste coordinate for cut text above
             COORD coordDestination;
             coordDestination.X = 0;
@@ -2044,6 +2049,11 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
         srScroll.Right = SHORT_MAX;
         srScroll.Top = cursorPosition.Y;
         srScroll.Bottom = screenInfo.GetViewport().BottomInclusive();
+        // Clip to the DECSTBM margin boundary
+        if (screenInfo.AreMarginsSet())
+        {
+            srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive();
+        }
         // Paste coordinate for cut text above
         COORD coordDestination;
         coordDestination.X = 0;

src/host/output.cpp

@@ -361,19 +361,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,
     // If there was no clip rect, we'll clip to the entire buffer size.
     auto clip = Viewport::FromInclusive(clipRectGiven.value_or(buffer.ToInclusive()));
 
-    // Account for the scroll margins set by DECSTBM
-    // DECSTBM command can sometimes apply a clipping behavior as well. Check if we have any
-    // margins defined by DECSTBM and further restrict the clipping area here.
-    if (screenInfo.AreMarginsSet())
-    {
-        const auto margin = screenInfo.GetScrollingRegion();
-
-        // Update the clip rectangle to only include the area that is also in the margin.
-        clip = Viewport::Intersect(clip, margin);
-
-        // We'll also need to update the source rectangle, but we need to do that later.
-    }
-
     // OK, make sure that the clip rectangle also fits inside the buffer
     clip = Viewport::Intersect(buffer, clip);
 
@@ -416,18 +403,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,
         targetOrigin.Y += currentSourceOrigin.Y - originalSourceOrigin.Y;
     }
 
-    // See MSFT:20204600 - Update the source rectangle to only include the region
-    //      inside the scroll margins. We need to do this AFTER we calculate the
-    //      delta between the currentSourceOrigin and the originalSourceOrigin.
-    // Don't combine this with the above block, because if there are margins set
-    //      and the source rectangle was clipped by the buffer, we still want to
-    //      adjust the target origin point based on the clipping of the buffer.
-    if (screenInfo.AreMarginsSet())
-    {
-        const auto margin = screenInfo.GetScrollingRegion();
-        source = Viewport::Intersect(source, margin);
-    }
-
     // And now the target viewport is the same size as the source viewport but at the different position.
     auto target = Viewport::FromDimensions(targetOrigin, source.Dimensions());
 

src/host/settings.cpp

@@ -326,6 +326,24 @@ void Settings::Validate()
     WI_ClearAllFlags(_wFillAttribute, ~(FG_ATTRS | BG_ATTRS));
     WI_ClearAllFlags(_wPopupFillAttribute, ~(FG_ATTRS | BG_ATTRS));
 
+    // If the extended color options are set to invalid values (all the same color), reset them.
+    if (_CursorColor != Cursor::s_InvertCursorColor && _CursorColor == _DefaultBackground)
+    {
+        _CursorColor = Cursor::s_InvertCursorColor;
+    }
+
+    if (_DefaultForeground != INVALID_COLOR && _DefaultForeground == _DefaultBackground)
+    {
+        // INVALID_COLOR is used as an "unset" sentinel in future attribute functions.
+        _DefaultForeground = _DefaultBackground = INVALID_COLOR;
+        // If the damaged settings _further_ propagated to the default fill attribute, fix it.
+        if (_wFillAttribute == 0)
+        {
+            // These attributes were taken from the Settings ctor and equal "gray on black"
+            _wFillAttribute = FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE;
+        }
+    }
+
     FAIL_FAST_IF(!(_dwWindowSize.X > 0));
     FAIL_FAST_IF(!(_dwWindowSize.Y > 0));
     FAIL_FAST_IF(!(_dwScreenBufferSize.X > 0));

src/host/srvinit.cpp

@@ -139,6 +139,17 @@ static bool s_IsOnDesktop()
             reg.LoadFromRegistry(Title);
         }
     }
+    else
+    {
+        // microsoft/terminal#1965 - Let's just always enable VT processing by
+        // default for conpty clients. This prevents peculiar differences in
+        // behavior between conhost and terminal applications when the user has
+        // VirtualTerminalLevel=1 in their registry.
+        // 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);
+    }
 
     // 1. The settings we were passed contains STARTUPINFO structure settings to be applied last.
     settings.ApplyStartupInfo(pStartupSettings);

src/host/ut_host/ApiRoutinesTests.cpp

@@ -594,9 +594,12 @@ class ApiRoutinesTests
     TEST_METHOD(ApiScrollConsoleScreenBufferW)
     {
         BEGIN_TEST_METHOD_PROPERTIES()
+            TEST_METHOD_PROPERTY(L"data:setMargins", L"{false, true}")
             TEST_METHOD_PROPERTY(L"data:checkClipped", L"{false, true}")
         END_TEST_METHOD_PROPERTIES();
 
+        bool setMargins;
+        VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins), L"Get whether or not we should set the DECSTBM margins.");
         bool checkClipped;
         VERIFY_SUCCEEDED(TestData::TryGetValue(L"checkClipped", checkClipped), L"Get whether or not we should check all the options using a clipping rectangle.");
 
@@ -605,6 +608,13 @@ class ApiRoutinesTests
 
         VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional({ 5, 5 }), L"Make the buffer small so this doesn't take forever.");
 
+        // Tests are run both with and without the DECSTBM margins set. This should not alter
+        // the results, since ScrollConsoleScreenBuffer should not be affected by VT margins.
+        auto& stateMachine = si.GetStateMachine();
+        stateMachine.ProcessString(setMargins ? L"\x1b[2;4r" : L"\x1b[r");
+        // 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"); });
+
         gci.LockConsole();
         auto Unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
 

src/host/ut_host/ScreenBufferTests.cpp

@@ -3282,6 +3282,13 @@ void ScreenBufferTests::ScrollOperations()
 
 void ScreenBufferTests::InsertChars()
 {
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}")
+    END_TEST_METHOD_PROPERTIES();
+
+    bool setMargins;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins));
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
     auto& stateMachine = si.GetStateMachine();
@@ -3295,6 +3302,12 @@ void ScreenBufferTests::InsertChars()
     VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false));
     si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true);
 
+    // Tests are run both with and without the DECSTBM margins set. This should not alter
+    // the results, since the ICH operation is not affected by vertical margins.
+    stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r");
+    // 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"); });
+
     Log::Comment(
         L"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. "
         L"Then insert 5 spaces at the cursor. Watch spaces get inserted, text slides right "
@@ -3425,6 +3438,13 @@ void ScreenBufferTests::InsertChars()
 
 void ScreenBufferTests::DeleteChars()
 {
+    BEGIN_TEST_METHOD_PROPERTIES()
+        TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}")
+    END_TEST_METHOD_PROPERTIES();
+
+    bool setMargins;
+    VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins));
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
     auto& stateMachine = si.GetStateMachine();
@@ -3438,6 +3458,12 @@ void ScreenBufferTests::DeleteChars()
     VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false));
     si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true);
 
+    // Tests are run both with and without the DECSTBM margins set. This should not alter
+    // the results, since the DCH operation is not affected by vertical margins.
+    stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r");
+    // 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"); });
+
     Log::Comment(
         L"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. "
         L"Then delete 5 characters at the cursor. Watch the rest of the line slide left, "

src/interactivity/win32/find.cpp

@@ -23,13 +23,16 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l
     // This bool is used to track which option - up or down - was used to perform the last search. That way, the next time the
     //   find dialog is opened, it will default to the last used option.
     static bool fFindSearchUp = true;
+    static std::wstring lastFindString;
+
     WCHAR szBuf[SEARCH_STRING_LENGTH + 1];
     switch (Message)
     {
     case WM_INITDIALOG:
         SetWindowLongPtrW(hWnd, DWLP_USER, lParam);
         SendDlgItemMessageW(hWnd, ID_CONSOLE_FINDSTR, EM_LIMITTEXT, ARRAYSIZE(szBuf) - 1, 0);
         CheckRadioButton(hWnd, ID_CONSOLE_FINDUP, ID_CONSOLE_FINDDOWN, (fFindSearchUp ? ID_CONSOLE_FINDUP : ID_CONSOLE_FINDDOWN));
+        SetDlgItemText(hWnd, ID_CONSOLE_FINDSTR, lastFindString.c_str());
         return TRUE;
     case WM_COMMAND:
     {
@@ -40,6 +43,7 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l
             USHORT const StringLength = (USHORT)GetDlgItemTextW(hWnd, ID_CONSOLE_FINDSTR, szBuf, ARRAYSIZE(szBuf));
             if (StringLength == 0)
             {
+                lastFindString.clear();
                 break;
             }
             bool const IgnoreCase = IsDlgButtonChecked(hWnd, ID_CONSOLE_FINDCASE) == 0;
@@ -48,7 +52,7 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l
             SCREEN_INFORMATION& ScreenInfo = gci.GetActiveOutputBuffer();
 
             std::wstring wstr(szBuf, StringLength);
-
+            lastFindString = wstr;
             LockConsole();
             auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
 

src/renderer/base/renderer.cpp

@@ -10,6 +10,8 @@
 using namespace Microsoft::Console::Render;
 using namespace Microsoft::Console::Types;
 
+static constexpr auto maxRetriesForRenderEngine = 3;
+
 // Routine Description:
 // - Creates a new renderer controller for a console.
 // Arguments:
@@ -62,7 +64,21 @@ Renderer::~Renderer()
 
     for (IRenderEngine* const pEngine : _rgpEngines)
     {
-        LOG_IF_FAILED(_PaintFrameForEngine(pEngine));
+        auto tries = maxRetriesForRenderEngine;
+        while (tries > 0)
+        {
+            const auto hr = _PaintFrameForEngine(pEngine);
+            if (E_PENDING == hr)
+            {
+                if (--tries == 0)
+                {
+                    FAIL_FAST_HR_MSG(E_UNEXPECTED, "A rendering engine required too many retries.");
+                }
+                continue;
+            }
+            LOG_IF_FAILED(hr);
+            break;
+        }
     }
 
     return S_OK;

src/renderer/dx/DxRenderer.cpp

@@ -866,15 +866,31 @@ void DxEngine::_InvalidOr(RECT rc) noexcept
 // Arguments:
 // - <none>
 // Return Value:
-// - S_OK or relevant DirectX error
+// - S_OK on success, E_PENDING to indicate a retry or a relevant DirectX error
 [[nodiscard]] HRESULT DxEngine::Present() noexcept
 {
     if (_presentReady)
     {
         try
         {
-            FAIL_FAST_IF_FAILED(_dxgiSwapChain->Present(1, 0));
-            /*FAIL_FAST_IF_FAILED(_dxgiSwapChain->Present1(1, 0, &_presentParams));*/
+            HRESULT hr = S_OK;
+
+            hr = _dxgiSwapChain->Present(1, 0);
+            /*hr = _dxgiSwapChain->Present1(1, 0, &_presentParams);*/
+
+            if (FAILED(hr))
+            {
+                // These two error codes are indicated for destroy-and-recreate
+                if (hr == DXGI_ERROR_DEVICE_REMOVED || hr == DXGI_ERROR_DEVICE_RESET)
+                {
+                    // We don't need to end painting here, as the renderer has done it for us.
+                    _ReleaseDeviceResources();
+                    FAIL_FAST_IF_FAILED(InvalidateAll());
+                    return E_PENDING; // Indicate a retry to the renderer.
+                }
+
+                FAIL_FAST_HR(hr);
+            }
 
             RETURN_IF_FAILED(_CopyFrontToBack());
             _presentReady = false;

src/renderer/vt/paint.cpp

@@ -475,14 +475,22 @@ using namespace Microsoft::Console::Types;
 
     if (useEraseChar)
     {
-        RETURN_IF_FAILED(_EraseCharacter(sNumSpaces));
         // ECH doesn't actually move the cursor itself. However, we think that
         //   the cursor *should* be at the end of the area we just erased. Stash
         //   that position as our new deferred position. If we don't move the
         //   cursor somewhere else before the end of the frame, we'll move the
         //   cursor to the deferred position at the end of the frame, or right
         //   before we need to print new text.
         _deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y };
+
+        if (_deferredCursorPos.X < _lastViewport.RightInclusive())
+        {
+            RETURN_IF_FAILED(_EraseCharacter(sNumSpaces));
+        }
+        else
+        {
+            RETURN_IF_FAILED(_EraseLine());
+        }
     }
     else if (_newBottomLine)
     {