PRE-MERGE #17143 Improve Viewport and Viewport::WalkInBounds

src/buffer/out/textBuffer.cpp

@@ -993,7 +993,7 @@ const til::CoordType TextBuffer::GetFirstRowIndex() const noexcept
 
 const Viewport TextBuffer::GetSize() const noexcept
 {
-    return Viewport::FromDimensions({ _width, _height });
+    return Viewport::FromDimensions({}, { _width, _height });
 }
 
 void TextBuffer::_SetFirstRowIndex(const til::CoordType FirstRowIndex) noexcept
@@ -1597,7 +1597,7 @@ til::point TextBuffer::_GetWordEndForSelection(const til::point target, const st
             }
         }
 
-        bufferSize.IncrementInBoundsCircular(result);
+        bufferSize.IncrementInBounds(result);
     }
 
     if (_GetDelimiterClassAt(result, wordDelimiters) != initialDelimiter)

src/cascadia/TerminalControl/ControlCore.cpp

@@ -2645,21 +2645,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // Select the region of text between [s.start, s.end), in buffer space
     void ControlCore::_selectSpan(til::point_span s)
     {
-        // s.end is an _exclusive_ point. We need an inclusive one. But
-        // decrement in bounds wants an inclusive one. If you pass an exclusive
-        // one, then it might assert at you for being out of bounds. So we also
-        // take care of the case that the end point is outside the viewport
-        // manually.
+        // s.end is an _exclusive_ point. We need an inclusive one.
         const auto bufferSize{ _terminal->GetTextBuffer().GetSize() };
         til::point inclusiveEnd = s.end;
-        if (s.end.x == bufferSize.Width())
-        {
-            inclusiveEnd = til::point{ std::max(0, s.end.x - 1), s.end.y };
-        }
-        else
-        {
-            bufferSize.DecrementInBounds(inclusiveEnd);
-        }
+        bufferSize.DecrementInBounds(inclusiveEnd);
 
         _terminal->SelectNewRegion(s.start, inclusiveEnd);
         _renderer->TriggerSelection();

src/cascadia/TerminalControl/HwndTerminal.cpp

@@ -356,7 +356,7 @@ HRESULT HwndTerminal::Refresh(const til::size windowSize, _Out_ til::size* dimen
     _renderer->TriggerRedrawAll();
 
     // Convert our new dimensions to characters
-    const auto viewInPixels = Viewport::FromDimensions(windowSize);
+    const auto viewInPixels = Viewport::FromDimensions({}, windowSize);
     const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels);
 
     // Guard against resizing the window to 0 columns/rows, which the text buffer classes don't really support.
@@ -464,7 +464,7 @@ try
 
     Viewport viewInPixels;
     {
-        const auto viewInCharacters = Viewport::FromDimensions(dimensionsInCharacters);
+        const auto viewInCharacters = Viewport::FromDimensions({}, dimensionsInCharacters);
         const auto lock = publicTerminal->_terminal->LockForReading();
         viewInPixels = publicTerminal->_renderEngine->GetViewportInPixels(viewInCharacters);
     }
@@ -491,7 +491,7 @@ try
 {
     const auto publicTerminal = static_cast<const HwndTerminal*>(terminal);
 
-    const auto viewInPixels = Viewport::FromDimensions({ width, height });
+    const auto viewInPixels = Viewport::FromDimensions({}, { width, height });
     const auto lock = publicTerminal->_terminal->LockForReading();
     const auto viewInCharacters = publicTerminal->_renderEngine->GetViewportInCharacters(viewInPixels);
 

src/cascadia/TerminalCore/Terminal.cpp

@@ -961,7 +961,7 @@ Viewport Terminal::_GetMutableViewport() const noexcept
     // GH#3493: if we're in the alt buffer, then it's possible that the mutable
     // viewport's size hasn't been updated yet. In that case, use the
     // temporarily stashed _altBufferSize instead.
-    return _inAltBuffer() ? Viewport::FromDimensions(_altBufferSize) :
+    return _inAltBuffer() ? Viewport::FromDimensions({}, _altBufferSize) :
                             _mutableViewport;
 }
 

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -495,7 +495,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir)
             searchEnd = { bufferSize.RightInclusive(), searchStart.y - 1 };
             searchStart = { bufferSize.Left(), std::max(searchStart.y - viewportHeight, bufferSize.Top()) };
         }
-        searchArea = Viewport::FromDimensions(searchStart, searchEnd.x + 1, searchEnd.y + 1);
+        searchArea = Viewport::FromDimensions(searchStart, { searchEnd.x + 1, searchEnd.y + 1 });
 
         const til::point bufferStart{ bufferSize.Origin() };
         const til::point bufferEnd{ bufferSize.RightInclusive(), ViewEndIndex() };
@@ -516,7 +516,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir)
                     searchEnd.y -= 1;
                     searchStart.y = std::max(searchEnd.y - viewportHeight, bufferSize.Top());
                 }
-                searchArea = Viewport::FromDimensions(searchStart, searchEnd.x + 1, searchEnd.y + 1);
+                searchArea = Viewport::FromDimensions(searchStart, { searchEnd.x + 1, searchEnd.y + 1 });
             }
         }
 

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -4873,13 +4873,7 @@ void ConptyRoundtripTests::ReflowPromptRegions()
             til::point afterPos = originalPos;
             // walk that original pos dx times into the actual real place in the buffer.
             auto bufferViewport = tb.GetSize();
-            const auto walkDir = Viewport::WalkDir{ dx < 0 ? Viewport::XWalk::LeftToRight : Viewport::XWalk::RightToLeft,
-                                                    dx < 0 ? Viewport::YWalk::TopToBottom : Viewport::YWalk::BottomToTop };
-            for (auto i = 0; i < std::abs(dx); i++)
-            {
-                bufferViewport.WalkInBounds(afterPos,
-                                            walkDir);
-            }
+            bufferViewport.WalkInBounds(afterPos, -dx);
             const auto expectedOutputStart = !afterResize ?
                                                  originalPos : // printed exactly a row, so we're exactly below the prompt
                                                  afterPos;

src/host/VtIo.cpp

@@ -150,9 +150,7 @@ VtIo::VtIo() :
 
         if (IsValidHandle(_hOutput.get()))
         {
-            auto initialViewport = Viewport::FromDimensions({ 0, 0 },
-                                                            gci.GetWindowSize().width,
-                                                            gci.GetWindowSize().height);
+            auto initialViewport = Viewport::FromDimensions({ 0, 0 }, gci.GetWindowSize());
             switch (_IoMode)
             {
             case VtIoMode::XTERM_256:

src/host/_output.cpp

@@ -223,7 +223,7 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
         {
             // Notify accessibility
             auto endingCoordinate = startingCoordinate;
-            bufferSize.MoveInBounds(cellsModifiedCoord, endingCoordinate);
+            bufferSize.WalkInBounds(endingCoordinate, cellsModifiedCoord);
             screenBuffer.NotifyAccessibilityEventing(startingCoordinate.x, startingCoordinate.y, endingCoordinate.x, endingCoordinate.y);
         }
     }
@@ -287,7 +287,7 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
         if (screenInfo.HasAccessibilityEventing())
         {
             auto endingCoordinate = startingCoordinate;
-            bufferSize.MoveInBounds(cellsModifiedCoord, endingCoordinate);
+            bufferSize.WalkInBounds(endingCoordinate, cellsModifiedCoord);
             screenInfo.NotifyAccessibilityEventing(startingCoordinate.x, startingCoordinate.y, endingCoordinate.x, endingCoordinate.y);
         }
 

src/host/outputStream.cpp

@@ -321,7 +321,7 @@ bool ConhostInternalGetSet::ResizeWindow(const til::CoordType sColumns, const ti
     api->GetConsoleScreenBufferInfoExImpl(screenInfo, csbiex);
 
     const auto oldViewport = screenInfo.GetVirtualViewport();
-    auto newViewport = Viewport::FromDimensions(oldViewport.Origin(), sColumns, sRows);
+    auto newViewport = Viewport::FromDimensions(oldViewport.Origin(), { sColumns, sRows });
     // Always resize the width of the console
     csbiex.dwSize.X = gsl::narrow_cast<short>(sColumns);
     // Only set the screen buffer's height if it's currently less than

src/host/screenInfo.cpp

@@ -163,7 +163,7 @@ Viewport SCREEN_INFORMATION::GetTerminalBufferSize() const
     auto v = _textBuffer->GetSize();
     if (gci.IsTerminalScrolling() && v.Height() > _virtualBottom)
     {
-        v = Viewport::FromDimensions({ 0, 0 }, v.Width(), _virtualBottom + 1);
+        v = Viewport::FromDimensions({}, { v.Width(), _virtualBottom + 1 });
     }
     return v;
 }

src/host/selectionInput.cpp

@@ -955,7 +955,7 @@ bool Selection::_HandleMarkModeSelectionNav(const INPUT_KEY_INFO* const pInputKe
             if (pcoordInputEnd != nullptr)
             {
                 // - 1 so the coordinate is on top of the last position of the text, not one past it.
-                gci.GetActiveOutputBuffer().GetBufferSize().MoveInBounds(-1, boundaries.end);
+                gci.GetActiveOutputBuffer().GetBufferSize().WalkInBounds(boundaries.end, -1);
                 *pcoordInputEnd = boundaries.end;
             }
             return true;

src/host/ut_host/ScreenBufferTests.cpp

@@ -79,7 +79,7 @@ class ScreenBufferTests
         VERIFY_SUCCEEDED(currentBuffer.SetViewportOrigin(true, { 0, 0 }, true));
         // Make sure the viewport always starts off at the default size.
         auto defaultSize = til::size{ CommonState::s_csWindowWidth, CommonState::s_csWindowHeight };
-        currentBuffer.SetViewport(Viewport::FromDimensions(defaultSize), true);
+        currentBuffer.SetViewport(Viewport::FromDimensions({}, defaultSize), true);
         VERIFY_ARE_EQUAL(til::point(0, 0), currentBuffer.GetTextBuffer().GetCursor().GetPosition());
         // Make sure the virtual bottom is correctly positioned.
         currentBuffer.UpdateBottom();

src/host/ut_host/TextBufferTests.cpp

@@ -3013,13 +3013,7 @@ void TextBufferTests::ReflowPromptRegions()
             til::point afterPos = originalPos;
             // walk that original pos dx times into the actual real place in the buffer.
             auto bufferViewport = tb.GetSize();
-            const auto walkDir = Viewport::WalkDir{ dx < 0 ? Viewport::XWalk::LeftToRight : Viewport::XWalk::RightToLeft,
-                                                    dx < 0 ? Viewport::YWalk::TopToBottom : Viewport::YWalk::BottomToTop };
-            for (auto i = 0; i < std::abs(dx); i++)
-            {
-                bufferViewport.WalkInBounds(afterPos,
-                                            walkDir);
-            }
+            bufferViewport.WalkInBounds(afterPos, -dx);
             const auto expectedOutputStart = !afterResize ?
                                                  originalPos : // printed exactly a row, so we're exactly below the prompt
                                                  afterPos;

src/host/ut_host/ViewportTests.cpp

@@ -109,7 +109,7 @@ class ViewportTests
         dimensions.width = rect.right - rect.left + 1;
         dimensions.height = rect.bottom - rect.top + 1;
 
-        const auto v = Viewport::FromDimensions(origin, dimensions.width, dimensions.height);
+        const auto v = Viewport::FromDimensions(origin, dimensions);
 
         VERIFY_ARE_EQUAL(rect.left, v.Left());
         VERIFY_ARE_EQUAL(rect.right, v.RightInclusive());
@@ -169,7 +169,7 @@ class ViewportTests
         dimensions.width = rect.right - rect.left + 1;
         dimensions.height = rect.bottom - rect.top + 1;
 
-        const auto v = Viewport::FromDimensions(dimensions);
+        const auto v = Viewport::FromDimensions({}, dimensions);
 
         VERIFY_ARE_EQUAL(rect.left, v.Left());
         VERIFY_ARE_EQUAL(rect.right, v.RightInclusive());
@@ -183,28 +183,6 @@ class ViewportTests
         VERIFY_ARE_EQUAL(dimensions, v.Dimensions());
     }
 
-    TEST_METHOD(CreateFromCoord)
-    {
-        til::point origin;
-        origin.x = 12;
-        origin.y = 24;
-
-        const auto v = Viewport::FromCoord(origin);
-
-        VERIFY_ARE_EQUAL(origin.x, v.Left());
-        VERIFY_ARE_EQUAL(origin.x, v.RightInclusive());
-        VERIFY_ARE_EQUAL(origin.x + 1, v.RightExclusive());
-        VERIFY_ARE_EQUAL(origin.y, v.Top());
-        VERIFY_ARE_EQUAL(origin.y, v.BottomInclusive());
-        VERIFY_ARE_EQUAL(origin.y + 1, v.BottomExclusive());
-        VERIFY_ARE_EQUAL(1, v.Height());
-        VERIFY_ARE_EQUAL(1, v.Width());
-        VERIFY_ARE_EQUAL(origin, v.Origin());
-        // clang-format off
-        VERIFY_ARE_EQUAL(til::size(1, 1), v.Dimensions());
-        // clang-format on
-    }
-
     TEST_METHOD(IsInBoundsCoord)
     {
         til::inclusive_rect r;
@@ -503,51 +481,6 @@ class ViewportTests
         VERIFY_ARE_EQUAL(screen.y, edges.bottom);
     }
 
-    TEST_METHOD(IncrementInBoundsCircular)
-    {
-        auto success = false;
-
-        til::inclusive_rect edges;
-        edges.left = 10;
-        edges.right = 19;
-        edges.top = 20;
-        edges.bottom = 29;
-
-        const auto v = Viewport::FromInclusive(edges);
-        til::point original;
-        til::point screen;
-
-        // #1 coord inside region
-        original.x = screen.x = 15;
-        original.y = screen.y = 25;
-
-        success = v.IncrementInBoundsCircular(screen);
-
-        VERIFY_IS_TRUE(success);
-        VERIFY_ARE_EQUAL(screen.x, original.x + 1);
-        VERIFY_ARE_EQUAL(screen.y, original.y);
-
-        // #2 coord right edge, not bottom
-        original.x = screen.x = edges.right;
-        original.y = screen.y = 25;
-
-        success = v.IncrementInBoundsCircular(screen);
-
-        VERIFY_IS_TRUE(success);
-        VERIFY_ARE_EQUAL(screen.x, edges.left);
-        VERIFY_ARE_EQUAL(screen.y, original.y + 1);
-
-        // #3 coord right edge, bottom
-        original.x = screen.x = edges.right;
-        original.y = screen.y = edges.bottom;
-
-        success = v.IncrementInBoundsCircular(screen);
-
-        VERIFY_IS_FALSE(success);
-        VERIFY_ARE_EQUAL(screen.x, edges.left);
-        VERIFY_ARE_EQUAL(screen.y, edges.top);
-    }
-
     TEST_METHOD(DecrementInBounds)
     {
         auto success = false;
@@ -593,52 +526,7 @@ class ViewportTests
         VERIFY_ARE_EQUAL(screen.y, edges.top);
     }
 
-    TEST_METHOD(DecrementInBoundsCircular)
-    {
-        auto success = false;
-
-        til::inclusive_rect edges;
-        edges.left = 10;
-        edges.right = 19;
-        edges.top = 20;
-        edges.bottom = 29;
-
-        const auto v = Viewport::FromInclusive(edges);
-        til::point original;
-        til::point screen;
-
-        // #1 coord inside region
-        original.x = screen.x = 15;
-        original.y = screen.y = 25;
-
-        success = v.DecrementInBoundsCircular(screen);
-
-        VERIFY_IS_TRUE(success);
-        VERIFY_ARE_EQUAL(screen.x, original.x - 1);
-        VERIFY_ARE_EQUAL(screen.y, original.y);
-
-        // #2 coord left edge, not top
-        original.x = screen.x = edges.left;
-        original.y = screen.y = 25;
-
-        success = v.DecrementInBoundsCircular(screen);
-
-        VERIFY_IS_TRUE(success);
-        VERIFY_ARE_EQUAL(screen.x, edges.right);
-        VERIFY_ARE_EQUAL(screen.y, original.y - 1);
-
-        // #3 coord left edge, top
-        original.x = screen.x = edges.left;
-        original.y = screen.y = edges.top;
-
-        success = v.DecrementInBoundsCircular(screen);
-
-        VERIFY_IS_FALSE(success);
-        VERIFY_ARE_EQUAL(screen.x, edges.right);
-        VERIFY_ARE_EQUAL(screen.y, edges.bottom);
-    }
-
-    til::CoordType RandomCoord()
+    static til::CoordType RandomCoord()
     {
         til::CoordType s;
 
@@ -673,29 +561,16 @@ class ViewportTests
 
             auto sAddAmount = RandomCoord() % (sRowWidth * sRowWidth);
 
-            til::point coordFinal;
-            coordFinal.x = (coordPos.x + sAddAmount) % sRowWidth;
-            coordFinal.y = coordPos.y + ((coordPos.x + sAddAmount) / sRowWidth);
-
-            Log::Comment(String().Format(L"Add To Position: (%d, %d)  Amount to add: %d", coordPos.y, coordPos.x, sAddAmount));
-
-            // Movement result is expected to be true, unless there's an error.
-            auto fExpectedResult = true;
-
-            // if we've calculated past the final row, then the function will reset to the original position and the output will be false.
-            if (coordFinal.y >= sRowWidth)
-            {
-                coordFinal = coordPos;
-                fExpectedResult = false;
-            }
-
-            const bool fActualResult = v.MoveInBounds(sAddAmount, coordPos);
+            const til::point coord{
+                (coordPos.x + sAddAmount) % sRowWidth,
+                coordPos.y + ((coordPos.x + sAddAmount) / sRowWidth),
+            };
+            const auto coordClamped = std::clamp(coord, v.Origin(), v.BottomRightInclusive());
+            const auto fExpectedResult = coord == coordClamped;
+            const bool fActualResult = v.WalkInBounds(coordPos, sAddAmount);
 
             VERIFY_ARE_EQUAL(fExpectedResult, fActualResult);
-            VERIFY_ARE_EQUAL(coordPos.x, coordFinal.x);
-            VERIFY_ARE_EQUAL(coordPos.y, coordFinal.y);
-
-            Log::Comment(String().Format(L"Actual: (%d, %d) Expected: (%d, %d)", coordPos.y, coordPos.x, coordFinal.y, coordFinal.x));
+            VERIFY_ARE_EQUAL(coordPos, coordClamped);
         }
     }
 

src/renderer/base/renderer.cpp

@@ -274,7 +274,7 @@ void Renderer::TriggerRedraw(const Viewport& region)
 // - <none>
 void Renderer::TriggerRedraw(const til::point* const pcoord)
 {
-    TriggerRedraw(Viewport::FromCoord(*pcoord)); // this will notify to paint if we need it.
+    TriggerRedraw(Viewport::FromDimensions(*pcoord, { 1, 1 })); // this will notify to paint if we need it.
 }
 
 // Routine Description: