Introduce the concept of "selection spans" instead of "rects" by DHowett · Pull Request #17638 · microsoft/terminal

DHowett · 2024-07-31T22:11:49Z

Right now, selections are stored as a set of points (start, end, pivot)
and converted into rectangles based on the contents of the screen buffer
every single render pass.

They are also painted fairly late in the rendering cycle.

This pull request makes a few changes to how selections happen.

Selections are now requested earlier in the rendering pass, during the
creation of the RenderInfo, and stored as a span<point_span>. To
make that span work, the selection data must be durably stored
somewhere.

Therefore, Terminal and conhost now take advantage of their new
generational selection data (#17676) to create and cache arrays of
point_spans covering their selections.

Unlike selection rects, linear (non-box) selections take only a single
entry in the span set. Also unlike selection rects, but like search
highlights, selection spans use buffer coordinates.

In the future, AtlasEngine can use the information sent in during
PrepareRenderInfo to influence text rendering decisions such as the
foreground and background color for a grapheme cluster or the gridlines.

Unfortunately, GDI is stuck with the old selection rendering
implementation and therefore, we still need to produce selection rects
with viewport coordinates and filter them down to the render engine's
dirty region. This is because GDI wants to invert all on-screen
pixels in the selection range after the final render to make sure
sixels, pixels, and lines are all displayed in the appropriate vintage
style.

As part of this change, I've migrated conhost's search highlight to not
read text directly out of the buffer but instead use the new
CopyRequest added by @tusharsnx in #16377.

Many of the tests for conhost behavior had a copy of conhost's code in
them, which sorta begs the question. I migrated them to use hardcoded
expected values to better catch regressions.

In Terminal, the "SelectAfterScroll" ControlCore tests weren't testing
anything they said they were, so I made them do so.

src/host/ut_host/SelectionTests.cpp

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

src/renderer/base/renderer.cpp

In #17638, I am moving selection to an earlier phase of rendering (so that further phases can take it into account). Since I am drafting off the design of search highlights, one of the required changes is moving to passing `span`s of `point_span`s around to make selection effectively zero-copy. We can't easily have zero-copy selection propagation without caching, and we can't have caching without mandatory cache invalidation. This pull request moves both conhost and Terminal to use `til::generational` for all selection members that impact the ranges that would be produced from `GetSelectionRects`. This required a move from `std::optional<>` to a boolean to determine whether a selection was active in Terminal. We will no longer regenerate the selection rects from the selection anchors plus the text buffer *every single frame*. Apart from being annoying to read, there is one downside. If you begin a selection on a narrow character, _and that narrow character later turns into a wide character_, we will show it as half-selected. This should be a rare-enough case that we can accept it as a regression.

DHowett · 2024-07-31T22:14:25Z

src/cascadia/TerminalCore/Terminal.hpp

    };
-    std::optional<SelectionAnchors> _selection;
-    bool _blockSelection = false;
+    til::generational<SelectionAnchors> _selection{ til::generation_t{ 1 } };


Leonard said generation 0 would be OK. The first change to selection would set it to 1, and the default state is "no selection".

src/cascadia/TerminalCore/Terminal.hpp

carlos-zamora

Nice work! Love how much simpler it is now 😊

src/renderer/base/renderer.cpp

src/host/selectionInput.cpp

lhecker

The only thing I had in mind were things that wouldn't even qualify as nits. 😅 LGTM. ✅

DHowett · 2024-08-15T16:16:37Z

@carlos-zamora I made your const changes and I'm letting the build spin. I'm surprised Audit mode didn't yell at me for it?

DHowett · 2024-08-15T16:25:50Z

Diff from before the rebase:

diff --git a/src/host/selectionInput.cpp b/src/host/selectionInput.cpp
index 426214465..d6190a113 100644
--- a/src/host/selectionInput.cpp
+++ b/src/host/selectionInput.cpp
@@ -675,14 +675,14 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo)
         if (fShiftPressed)
         {
             // Search the selection and color *that*
-            auto req = TextBuffer::CopyRequest::FromConfig(textBuffer,
-                                                           til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top },
-                                                           til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom },
-                                                           true /* multi-line search doesn't make sense; concatenate all lines */,
-                                                           false /* we filtered out block search above */,
-                                                           true /* trim block selection */,
-                                                           true);
-            auto str = textBuffer.GetPlainText(req);
+            const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer,
+                                                                 til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top },
+                                                                 til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom },
+                                                                 true /* multi-line search doesn't make sense; concatenate all lines */,
+                                                                 false /* we filtered out block search above */,
+                                                                 true /* trim block selection */,
+                                                                 true);
+            const auto str = textBuffer.GetPlainText(req);
             // Clear the selection and call the search / mark function.
             ClearSelection();
 
diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp
index 4b4dcf2ce..33892087e 100644
--- a/src/renderer/base/renderer.cpp
+++ b/src/renderer/base/renderer.cpp
@@ -328,7 +328,7 @@ void Renderer::TriggerSelection()
 try
 {
     const auto spans = _pData->GetSelectionSpans();
-    if (spans.size() != _lastSelectionPaintSize || (spans.size() && _lastSelectionPaintSpan != til::point_span{ spans.front().start, spans.back().end }))
+    if (spans.size() != _lastSelectionPaintSize || (!spans.empty() && _lastSelectionPaintSpan != til::point_span{ spans.front().start, spans.back().end }))
     {
         std::vector<til::rect> newSelectionViewportRects;
 
@@ -338,7 +338,7 @@ try
             _lastSelectionPaintSpan = til::point_span{ spans.front().start, spans.back().end };
 
             const auto& buffer = _pData->GetTextBuffer();
-            auto bufferWidth = buffer.GetSize().Width();
+            const auto bufferWidth = buffer.GetSize().Width();
             const til::rect vp{ _viewport.ToExclusive() };
             for (auto&& sp : spans)
             {
diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp
index 8e5bfb824..155ea792a 100644
--- a/src/renderer/uia/UiaRenderer.cpp
+++ b/src/renderer/uia/UiaRenderer.cpp
@@ -20,7 +20,6 @@ UiaEngine::UiaEngine(IUiaEventDispatcher* dispatcher) :
     _textBufferChanged{ false },
     _cursorChanged{ false },
     _isEnabled{ true },
-    _prevSelection{},
     _prevCursorRegion{},
     RenderEngineBase()
 {
@@ -110,34 +109,13 @@ CATCH_RETURN();
 // - rectangles - One or more rectangles describing character positions on the grid
 // Return Value:
 // - S_OK
-[[nodiscard]] HRESULT UiaEngine::InvalidateSelection(std::span<const til::rect> rectangles) noexcept
-try
+[[nodiscard]] HRESULT UiaEngine::InvalidateSelection(std::span<const til::rect> /*rectangles*/) noexcept
 {
-    // early exit: different number of rows
-    if (_prevSelection.size() != rectangles.size())
-    {
-        _selectionChanged = true;
-        _prevSelection.assign(rectangles.begin(), rectangles.end());
-        return S_OK;
-    }
-
-    _selectionChanged = false; // assume they're the same
-
-    auto i = rectangles.begin();
-    auto j = _prevSelection.begin();
-    // safe to iterate j until i is exhausted because we checked their sizes
-    for (; i != rectangles.end(); ++i, ++j)
-    {
-        if (*i != *j)
-        {
-            _selectionChanged = true;
-            _prevSelection.assign(rectangles.begin(), rectangles.end());
-            break;
-        }
-    }
+    // INVARIANT: Renderer checks the incoming selection spans and only calls InvalidateSelection
+    // if they have actually changed.
+    _selectionChanged = true;
     return S_OK;
 }
-CATCH_LOG_RETURN_HR(E_FAIL);
 
 // Routine Description:
 // - Scrolls the existing dirty region (if it exists) and
diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp
index 8bdeb15f4..c6038751f 100644
--- a/src/renderer/uia/UiaRenderer.hpp
+++ b/src/renderer/uia/UiaRenderer.hpp
@@ -74,7 +74,6 @@ namespace Microsoft::Console::Render
 
         Microsoft::Console::Types::IUiaEventDispatcher* _dispatcher;
 
-        std::vector<til::rect> _prevSelection;
         til::rect _prevCursorRegion;
     };
 }

DHowett · 2024-08-15T16:48:24Z

I bet the changes to SetViewportPosition have broken the scroll test. Let me re-figure that one.

conhost: rewrite colorsearch to use GetPlainText and GetSelectionSpans I also removed a spurious GetSelectionRects in the clipboard code conhost: Get the tests working with GetSelectionSpans conhost: remove GetSelectionRects completely conhost (consider merging): use GetTextSpans

TerminalCore: Get the tests working with Selection spans

The renderer will consume *buffer-relative selection spans* and prepare them into *viewport-relative rects*. PaintSelection will still filter rects by whether they are in the dirty area.

DHowett · 2024-08-15T17:12:59Z

Latest push:

commit 492fb1b3744d3615cee7b04403617c49695d562a
Author: Dustin L. Howett <[email protected]>
Date:   Thu Aug 15 12:12:15 2024 -0500

    fixup! terminal: port TerminalSelection to GetSelectionSpans

diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
index 5b21b3e41..11f01c752 100644
--- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
@@ -259,8 +259,8 @@ namespace TerminalCoreUnitTests
         {
             Terminal term{ Terminal::TestDummyMarker{} };
             DummyRenderer renderer{ &term };
-            til::CoordType scrollbackLines = 5;
-            term.Create({ 100, 100 }, scrollbackLines, renderer);
+            til::CoordType scrollbackLines = 100;
+            term.Create({ 120, 30 }, scrollbackLines, renderer);
 
             const til::CoordType contentScrollLines = 15;
             // Simulate a content-initiated scroll down by 15 lines

…a overlay (#17725) With the merge of #17638, selections are now accumulated early in the rendering process. This allows Atlas, which currently makes decisions about cell foreground/background at the time of text rendering, awareness of the selection ranges *before* text rendering begins. As a result, we can now paint the selection into the background and foreground bitmaps. We no longer need to overlay a rectangle, or series of rectangles, on top of the rendering surface and alpha blend the selection color onto the final image. As a reminder, "alpha selection" was always a stopgap because we didn't have durable per-cell foreground and background customization in the original DxEngine. Selection foregrounds are not customizable, and will be chosen using the same color distancing algorithm as the cursor. We can make them customizable "easily" (once we figure out the schema for it) for #3580. `ATLAS_DEBUG_SHOW_DIRTY` was using the `Selection` shading type to draw colored regions. I didn't want to break that, so I elected to rename the `Selection` shading type to `FilledRect` and keep its value. It helps that the shader didn't have any special treatment for `SHADING_TYPE_SELECTION`. This fixes the entire category of issues created by selection being an 80%-opacity white rectangle. However, given that it changes the imputed colors of the text it will reveal `SGR 8` concealed/hidden characters. Refs #17355 Refs #14859 Refs #11181 Refs #8716 Refs #4971 Closes #3561

github-advanced-security bot found potential problems Jul 31, 2024

View reviewed changes

src/host/ut_host/SelectionTests.cpp Fixed Show fixed Hide fixed

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp Fixed Show fixed Hide fixed