Ensure the foreground highlight color is correctly applied to the search matches

[tusharsnx]

TBU

[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: https://github.com/microsoft/terminal/blob/0d47c862c2d8e4733ed8bcc6d57a90105d4d1712/src/tools/ConsoleMonitor/main.cpp#L101-L150

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]

[DHowett]

It looks great in your screenshots!

[tusharsnx]

highlighted regions are getting invalidated by the scrolling. Converting the PR to Draft while I investigate this.

[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)

[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;
        }
    }
}

[github-actions[bot]]

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

:x: Errors	Count
:x: forbidden-pattern	1
:x: ignored-expect-variant	3

See :x: Event descriptions for more information.

Previously acknowledged words that are now absent

AAAa Jsons vtapi 🫥

Available :books: dictionaries could cover words (expected and unrecognized) not in the :blue_book: dictionary

This includes both expected items (2228) from .github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt .github/actions/spelling/expect/alphabet.txt .github/actions/spelling/expect/expect.txt .github/actions/spelling/expect/web.txt and unrecognized words (0)

Dictionary	Entries	Covers	Uniquely
cspell:swift/src/swift.txt	53	1	1
cspell:gaming-terms/dict/gaming-terms.txt	59	1	1
cspell:monkeyc/src/monkeyc_keywords.txt	123	1	1
cspell:cryptocurrencies/cryptocurrencies.txt	125	1	1
cspell:scala/dict/scala.txt	153	1	1

Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

      with:
        extra_dictionaries:
          cspell:swift/src/swift.txt
          cspell:gaming-terms/dict/gaming-terms.txt
          cspell:monkeyc/src/monkeyc_keywords.txt
          cspell:cryptocurrencies/cryptocurrencies.txt
          cspell:scala/dict/scala.txt

To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

check_extra_dictionaries: ''

[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. 😣)