Improve Search Highlighting

[tusharsnx]

The changeset involves:

• Decoupling Selection and Search Highlighting code paths.
• We no longer invalidate search highlights when:
  • Left-clicking on terminal
  • A new selection is made
  • Left-clicking on Search-box
  • Dispatching Find Next/Prev Match Action. (The search highlight was removed after pressing the first key of the Action's key combination)
  • And, anything that doesn't change buffer content, shouldn't invalidate the highlighted region (E.g. Cursor movement)
• Highlighting foreground color is actually applied to the highlighted text.
• Double-clicking on SearchBox no longer starts a text selection in the terminal.
• Selected text is properly populated in the Search Box (Search box doesn't pick up the newly selected text from the buffer #16355)

Closes: #16355

Some Implementation Details

Detecting text layout changes in the Control layer

As Search Highlight regions need to be removed when new text is added, or the existing text is re-arranged due to window resize or similar events, a new event TextLayoutUpdated is added that notifies CoreControl of any text layout changes. The event is used to invalidate and remove all search highlight regions from the buffer (because the regions might not be fresh anymore.

The new event is raised when:

1. AdaptDispatch writes new text into the buffer.
2. MainBuffer is switched to AltBuffer or vice-versa.
3. The user resized the window.
4. Font size changed.
5. Zoom level changed.

(Intensionally,) It's not raised when:

1. Buffer is scrolled.
2. The text cursor is moved.

When ControlCore receives a TextLayoutUpdated event, it clears the Search Highlights in the render data, and raises an UpdateSearchResults event to notify TermControl to update the Search UI (SearchBoxControl).

In the future, we can use TextLayoutUpdated event to start a new search which would refresh the results automatically after a slight delay (throttled). VSCode already does this today.

How does AtlasEngine draw the highlighted regions?

We follow a similar idea as for drawing the Selection region. When new regions are available, the old+new regions are marked invalidated. Later, a call to _drawHighlighted() is made at the end of PaintBufferLine() to override the highlighted regions' colors with highlight colors. The highlighting colors replace the buffer colors while search highlights are active.

Note that to paint search highlights, we currently invalidate the row completely. This forces text shaping for the rows in the viewport that have at least one highlighted region. This is done to keep the (already lengthy) PR... simple. We could take advantage of the fact that only colors have changed and not the characters (or glyphs). I'm expecting that this could be improved like:

1. When search regions are added, we add the highlighting colors to the color bitmaps without causing text shaping.
2. When search regions are removed, we re-fill the color bitmaps with the original colors from the Buffer.

Validation Steps:

• New text, window resize, font size changes, zooming, and pasting content into the terminal removes search highlights.
• highlighting colors override the foreground and background color of the text (in the rendered output).
• Blinking, faded, reverse video, Intense text is highlighted as expected.

[lhecker]

Ah, I see now... I believe the correct fix would actually be to move the PaintSelections call from Renderer::_PaintSelection to (for instance) Renderer::_PaintBackground.

Also, I just noticed I really need to finish documenting all the AtlasEngine code... ShapedRow is quite poorly documented in particular.
The AtlasEngine code isn't cleaned up yet and so some ShapedRow members are still terminal-oriented (the gridline ranges and selections are in columns instead of in pixels). But in general, ShapedRow is capable of representing arbitrary text, even proportional fonts, because instead of characters it uses glyphs and instead of columns it uses pixels.
The ShapedRow::colors array associates a foreground color to each glyph in the ShapedRow::glyphIndices array. That's why your approach won't work: The glyphs in the array don't necessarily map 1:1 to the column indices.

---

Less important thoughts:

In my opinion the place we put the PaintSelections call in renderer.cpp doesn't really matter. I consider it a temporary solution until I (we?) find the time to rewrite the entire rendering code. My goal is to replace the IRenderEngine interface members with just a single virtual Render(Payload) = 0 function. The Payload would consist of a TextBuffer snapshot as well as the Settings struct you can find in AtlasEngine's common.h file. Something like the GDI rendering code would then look more like this:

terminal/src/tools/ConsoleMonitor/main.cpp

Lines 101 to 150 in 0d47c86

	for (SHORT y = 0; y < g_cellCount.Y; y++)
	{
	const auto ciOffset = g_bufferSize.X * y;
	for (SHORT x = 0, x2; x < g_cellCount.X; x = x2)
	{
	const auto& ci = g_buffer[ciOffset + x];
	const auto fg = g_info.ColorTable[(ci.Attributes >> 0) & 15];
	const auto bg = g_info.ColorTable[(ci.Attributes >> 4) & 15];
	g_text.clear();
	g_textAdvance.clear();
	g_text.push_back(ci.Char.UnicodeChar);
	g_textAdvance.push_back(g_cellSize.cx);
	// Accumulate characters and advance widths until either the foreground or background
	// color changes. It also handles joining wide glyphs in a somewhat poor manner.
	for (x2 = x + 1; x2 < g_cellCount.X; x2++)
	{
	const auto& ci2 = g_buffer[ciOffset + x2];
	const auto fg2 = g_info.ColorTable[(ci2.Attributes >> 0) & 15];
	const auto bg2 = g_info.ColorTable[(ci2.Attributes >> 4) & 15];
	if (fg2 != fg || bg2 != bg)
	{
	break;
	}
	if (ci2.Attributes & COMMON_LVB_TRAILING_BYTE)
	{
	g_textAdvance.back() *= 2;
	}
	else
	{
	g_text.push_back(ci2.Char.UnicodeChar);
	g_textAdvance.push_back(g_cellSize.cx);
	}
	}
	RECT r;
	r.left = g_cellSize.cx * x;
	r.top = g_cellSize.cy * y;
	r.right = g_cellSize.cx * x2;
	r.bottom = r.top + g_cellSize.cy;
	SetTextColor(dc.get(), fg);
	SetBkColor(dc.get(), bg);
	ExtTextOutW(dc.get(), r.left, r.top, ETO_CLIPPED, &r, g_text.data(), static_cast<UINT>(g_text.size()), g_textAdvance.data());
	}
	}

I've put that idea on pause for now though, because I also want to replace VtEngine with a direct console API -> VT translation at some point. Doing that first would mean that I need to rewrite one less engine. Also, the UIA engine could probably also be removed as an engine. If we write a super simple "VT-sequence + control code" stripper function we could feed the input text directly into a UIA's NotifyNewOutput. The other 3 UIA functions are also very simple to implement without hooking into the rendering code. This would leave just the plain text renderers, of which only 1 can be active at any given time, which will probably allow us to remove even more code in various places. I'm confident that this project will fit on a postcard one day. As it should. 😄

[DHowett]

@lhecker is this a request for a change, or a "i will sign off and we can fix it more later"?

[tusharsnx]

@DHowett I'm still working on the search highlight right now. I'll definitely have something by tomorrow. I'm planning to repurpose this PR for that.

Just a note on what I'm working on:

Planning to decouple our Selection logic from Search highlighting. You can expect it to be like "find on page" in browsers. Starting a new selection while search highlight is active won't clear the search highlight 🙂

---

@lhecker as you mentioned, the current approach won't work properly due to M:N character to glyph relation. I think we can use clusterMap for figuring out what range of glyphs belong to the text within from/to. I already have something that works (almost).

[lhecker]

Hmm... Is it really necessary to use the cluster map at all? Any modifications we make to the foreground color bitmap before PaintBufferLine gets called will end up colorizing the glyphs. I haven't debugged it myself yet, but as far as I can see (or rather: theorize) this issue happens because highlights get drawn after the calls to PaintBufferLine and this results in the _p.colorBitmap changes to not have any effect. If we simply ensure that PaintSelections is called first, then it should work... I think.

[lhecker]

I'm not sure how obvious this since I've authored the code. So just to make sure I'll explain it: The purpose of the color bitmaps (both are in _p.colorBitmap) is to "decompress" the TextBuffer's viewport into a simple 2D matrix to make working with it simpler than with the run-length encoded TextAttributes. This makes scrolling much easier (a simple memmove) and also simplifies drawing with D2D and D3D (it simply maps/draws the bitmaps). It doesn't simplify _mapCharacters however, but it improves performance (we pass entire rows of text to DirectWrite at a time) and it avoided having to modify the IRenderEngine interface for now, which I plan on doing at some point in the future (hopefully).
But that's why you can basically draw anything into those bitmaps and have it take immediate effect, as long as you do it before PaintBufferLine gets called.

[tusharsnx]

Fixed the scroll invalidation issue. The Automation peer based TextChanged event didn't work out as I expected so I created a new TextLayoutUpdated event that only fires if the text is really changed in some way.

(I've updated the PR implementation details. Might be helpful for review)

[zadjii-msft]

Yea I'm cool with this. This is a PR that IMO is easiest to read from bottom-up.

One thing that might change here - I think currently you can open the find dialog, search for something, then hit esc and you've got that text selected. I don't think that'll work anymore, but I'm also not sure how niche that use case is.

[zadjii-msft]

note to self: okay so this still works in conhost, in GDI, same as before, because GDI is still just using the selection rect from _GetSelectionRects, and painting in the PaintSelection call on L1253

[zadjii-msft]

little sus that this is missing now... note to self, make sure that search marks still update when search box is open? ...

[zadjii-msft]

I answered this to myself: it's down in TermControl::_coreUpdateSearchResults

[zadjii-msft]

...... ah yea here it is. We always send the update if we got an explicit change to the search marks, which we will when the buffer is circling.

[lhecker]

I apologize for the late review. I was sort of hung up in graphemes and some other work.

To be honest, I have some concerns with this PR, primarily due to 2 reasons:

• There shouldn't be any setters on IRenderData.
• The performance could be significantly improved.

Here's why:

• There shouldn't be any setters on IRenderData.

I'm trying to implement buffer snapshotting. Additionally, the IRenderData interface incurs a high runtime cost by its very nature (virtual dispatch + control flow guard = bad) and prevents us from making some some major architectural improvements apart from buffer snapshotting (we can reduce AtlasEngine's code by probably half if we had direct access to the TextBuffer).

My long term vision is to have this:

// Just like AtlasEngine's Settings, and with the exact same idea, but cleaned up for general use.
// It's a snapshot of the Terminal's settings.
struct Settings {
    til::generational<TargetSettings> target;
    til::generational<FontSettings> font;
    til::generational<CursorSettings> cursor;
    til::generational<MiscellaneousSettings> misc;
    til::size targetSize;
    til::size viewportCellCount;
    til::point viewportOffset;
};

// Just like AtlasEngine's RenderingPayload, also with the exact same idea.
// It's a snapshot of the TextBuffer contents and terminal settings.
struct RenderingPayload {
   til::generational<Settings> s;
   TextBuffer buffer;
   std::vector<til::point_span> searchResults; // <---- your addition
};

struct IRenderData {
    void Snapshot(RenderingPayload& payload) = 0;
}

A terminal would then implement it as

void Terminal::Snapshot(RenderingPayload& p)
{
    if (p.s->targetSize != _windowSize) {
        p.s.write()->targetSize = _windowSize;
    }
    if (_searcher.HasChangedSinceLastTime()) {
        p.searchResults = _searcher.GetResults();
    }
    // Followed by a lot more settings update checks, which makes it very verbose. However, all those
    // checks will ultimately be extremely cheap overall, much cheaper than virtual function calls.
    // They'll also be easy to debug and easy to author exactly because they're so verbose.
    // I suspect the introduction of a couple helper methods will make it reasonable though.

    // Snapshot the buffer
    if (p.buffer.Size() != viewportSize) {
        p.buffer.Resize(viewportSize);
    }
    _activeBuffer.CopyTo(viewport, p.buffer);
}

This PR however moves further away from this goal, due to the two new setters.

I'd prefer if find.cpp's implementation instead was different from ControlCore.cpp's which would avoid the introduction of new interfaces. That's how it worked so far, because only ControlCore called HighlightResults().

You can add the two std::vector<til::inclusive_rect> members from Terminal straight to IRenderData as publicly accessible members if you want to and if it helps, since they would move to a RenderingPayload struct at some point anyway. Alternatively, you could keep the getters and simply remove the setters. This would be possible by returning the result list from the Search class and letting ControlCore call into Terminal directly.

If you need any help with this, please let me know. This is the only "blocker" I have for this PR. Anything else I'll say below is mostly a nitpick.

(If you plan to address any of the nitpicks, I personally think the one about the dirty rects is the only important one. Unless I'm mistaking, I believe it would simplify your PR a lot.)

---

• The performance could be significantly improved.

The transformation from point_span to inclusive_rect is fairly expensive, because GetTextRects is kinda "meh". If you look at its implementation you'll probably notice that it could be significantly improved by returning a small_vector, by not using GetCellDataAt (indirectly) and so on.

Additionally, filtering currently happens in Renderer and it uses dirty rects.

You could improve the performance a lot here by moving the transformation over into AtlasEngine. I think it should be something like this (untested pseudo code):

struct point_span
{
    til::point start;
    til::point end;

    // Calls func(row, begX, endX) for each row and begX and begY are inclusive coordinates,
    // because point_span itself also uses inclusive coordinates.
    // In other words, it turns a
    //   +----------------+
    //   |       #########|
    //   |################|
    //   |####            |
    //   +----------------+
    // into
    //   func(0, 8, 15)
    //   func(1, 0, 15)
    //   func(2, 0, 4)
    void iterate_rows(til::CoordType width, auto&& func) {
        // Copy the members so that the compiler knows it doesn't
        // need to re-read them on every loop iteration.
        const auto w = width - 1;
        const auto ax = std::clamp(start.x, 0, w);
        const auto ay = start.y;
        const auto bx = std::clamp(end.x, 0, w);
        const auto by = end.y;

        for (auto y = ay; y <= by; ++y) {
            const auto x1 = y != ay ? 0 : ax;
            const auto x2 = y != by ? w : bx;
            func(y, x1, x2);
        }
    }
};

Then you can iterate through the highlights directly, offset the y by the scroll offset and check if it's within AtlasEngine's viewport. IIRC we don't have access to the scroll/viewport offset, but it would probably be something we could pass through right? The iteration can be made faster by checking the start.y and end.y before calling iterate_rows in the first place.

For this to work you need access to all line renditions, and so I would suggest changing _api.lineRendition into a Buffer<LineRendition> with a height equal to the viewportCellCount (in _recreateCellCountDependentResources).

More importantly, I haven't quite understood whether the dirty rect manipulation is strictly necessary, but I suspect it's not. I would suggest to take the colorBitmap code from AtlasEngine::PaintBufferLine and extract it into _fillBackground/Foreground helper functions (or some similar name), then call those from both PaintBufferLine and from _drawHighlighted. The code in PaintBufferLine already takes care of figuring out whether there's a difference and marking the bitmap as dirty. It currently doesn't take care of updating the _p.dirtyRectInPx, because this happens implicitly: PaintBufferLine is called for each row marked by _p.invalidatedRows and in StartPaint we already mark that range as dirty in _p.dirtyRectInPx in advance. In other words, I would suggest something like this (pseudo code):

void AtlasEngine::_drawColorBitmap(size_t index, size_t y, size_t x1, size_t x2, u32 color) noexcept
{
    const auto shift = _api.lineRenditions[y] != LineRendition::SingleWidth ? 1 : 0;
    const auto row = _p.colorBitmap.begin() + _p.colorBitmapDepthStride * index + _p.colorBitmapRowStride * y;
    auto beg = row + (x1 << shift);
    auto end = row + (x2 << shift);

    for (auto it = beg; it != end; ++it)
    {
        if (*it != color)
        {
            _p.colorBitmapGenerations[index].bump();
            _p.dirtyRectInPx.left = 0;
            _p.dirtyRectInPx.right = _p.s->targetSize.x;
            _p.dirtyRectInPx.top = std::min(_p.dirtyRectInPx.top, gsl::narrow_cast<i32>(y * _p.s->font->cellSize.y));
            _p.dirtyRectInPx.bottom = std::max(_p.dirtyRectInPx.bottom, _p.dirtyRectInPx.top + _p.s->font->cellSize.y);
            std::fill(it, end, color);
            return;
        }
    }
}

[lhecker]

(As mentioned in the other PR, I'm currently hung up in finishing up a massive PR myself. I'll review your PR as soon as I'm done with that. Should be soon. 😣)

[lhecker]

nice

[DHowett]

hey @lhecker, wonder if we should put the pattern matcher update in here ;P

[DHowett]

what keeps these pointing to valid memory?

[lhecker]

My understanding was that these are only used to smuggle data between different IRenderEngine API calls only within a single render pass (since the interface is stateful and AtlasEngine is more "snapshot-y").

[tusharsnx]

_api.searchHighlights is always reset with the new value at the start of each render pass. The highlights are read and drawn all under the lock, and if I'm not wrong, we never reset highlights without holding a lock on Terminal.

I do think that we now need to be extra careful to always call ResetIfStale() under the lock, otherwise it could corrupt the AtlasEngine _api.searchHighlights. But, as @lhecker mentioned, in the future, the highlights will be stored as std::vector instead of std::span so there won't be any safety issues here.

[tusharsnx]

I think I should've at least reset the spans to be empty in EndPaint().

[DHowett]

this sounds expensive; is it?

[lhecker]

You're right, this is likely to be expensive, in particular in extreme cases like rainbowbench.

We could raise the event only once after processing an entire VT string to reduce the cost. One option would be to just do it as part of the updatePatternLocations flow, because that way it would also get throttled.

[tusharsnx]

We could raise the event only once after processing an entire VT string to reduce the cost.

That makes sense. We don't render in between parsing so doing it at every write step vs at the end of the entire vt string is equivalent.

I'm also planning to introduce throttling in this codepath, but I've a few queries like currently typing acts like a way for user to dismiss highlights from the screen. Once we introduce throttling this won't happen anymore. Instead typing into the terminal will dispatch a delayed refresh of search highlights which will bring back the highlights after a short delay. So, I'm not sure how should a user dismiss highlights in this case.

Any suggestions?

[lhecker]

Hmm, I may be misunderstanding you, but what you want is to simply remove the search highlights when someone types, right? That's basically just like TrySnapOnInput - if you search it in the project, there should only be a few hits. That function is what makes sure you're scrolled all the way down when you type, and we could just extend that, or add code near it, to also dismiss the highlights.

[tusharsnx]

what you want is to simply remove the search highlights when someone types, right?

Huh.. quite the opposite 😅 Highlights shouldn't be removed if search box is open. So, unless the searchbox is closed, we should keep the results fresh and active.

To be honest, I don't know what is a good UX here 😄

[lhecker]

Oh I see, now I got it. 🙂
In a perfect world we'd probably reflow the highlights while reflowing the text during a resize, but I don't think that's worth the effort at the moment. I think putting it into the throttled updatePatternLocations callback is probably the right choice for now as it presents a decent trade-off between correctness and complexity.

[lhecker]

FYI I just started making the necessary changes. 😊