Highlight all search results while the search box is open #16227

e82eric · 2023-10-25T14:50:28Z

Hi! First time contributor, wanted to try to add highlighting for all search results.

Summary of the Pull Request

Update to highlight all search results

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Follows the existing selection code as much as possible.
Updated logic that finds selection rectangles to also identify search rectangles.
Added structure to ShapedRow to track the search selections.

Validation Steps Performed

PR Checklist

Closes #xxx
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

e82eric · 2023-10-25T14:51:31Z

@microsoft-github-policy-service agree

microsoft#7561 Follows the existing selection code as much as possible.

lhecker

I'm a little concerned about how this works when you have >10k search results (which can easily happen if you search for a common letter like e). Preferably this PR should get more performance enhancements, but I don't consider them necessary to approve this PR. I can do them myself later on.

src/buffer/out/search.cpp

src/host/renderData.cpp

src/renderer/atlas/BackendD3D.cpp

src/renderer/atlas/AtlasEngine.cpp

This is the wrong place to put it, because conhost (aka OpenConsole) doesn't use this information. Additionally, this method runs every time you click "next" or "previous" so this would be a waste of compute since the _results array doesn't change. I would put this here: https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalControl/ControlCore.cpp#L1605C9-L1609 FYI never forget to std::move container classes whenever you pass them by value. Otherwise you make a copy just to throw away the original soon after.

y < _p.rows.size() will always be true due to the preceding clamp() on y. A better way to express this would be to put this at the start of the loop: if (rect.bottom <= 0 || rect.top >= _p.s->viewportCellCount.y) { continue; }

I don't think this is right. This returns the rectangles manually selected by the user and not the rectangles selected during a search. This should return nothing.

Fix for issue where search result is not highlighted when it starts on the first character of the line.

This code rejoins neighboring search results, but is that really such a common scenario that it outweighs the cost of turning this O(n) outer loop into O(n^2)? Previously we only had a 1 selection overlay per row at most (the regular, manual selection by a user). Now, with an arbitrary number of disjoint selections per row, the technically best approach is to resize RenderingPayload::colorBitmap to contain 3 textures and not just 2. Right now the bitmap contains the background and foreground bitmaps back to back, but it could also contain a bitmap for the selection. Then, in BackendD3D we simply overlay this bitmap on top of the previously drawn text, similar to how we draw the background as the first step. But making this change is fairly challenging and I don't expect you to make it.

check search rectangles before calling PaintSelections

This code rejoins neighboring search results, but is that really such a common scenario that it outweighs the cost of turning this O(n) outer loop into O(n^2)? Previously we only had a 1 selection overlay per row at most (the regular, manual selection by a user). Now, with an arbitrary number of disjoint selections per row, the technically best approach is to resize RenderingPayload::colorBitmap to contain 3 textures and not just 2. Right now the bitmap contains the background and foreground bitmaps back to back, but it could also contain a bitmap for the selection. Then, in BackendD3D we simply overlay this bitmap on top of the previously drawn text, similar to how we draw the background as the first step. But making this change is fairly challenging and I don't expect you to make it.

Removing more accidental white space

Fixed from Invoke-CodeFormat

e82eric · 2023-11-02T16:14:41Z

@lhecker just a quick heads up, I think this is ready for another round of reviews. (No rush just wasn't sure what the protocol for alerting you was).

lhecker · 2023-11-03T17:36:57Z

Honestly, after reading through it once, I couldn't find any other obvious flaws. If you don't mind, I'll wait a bit before I approve it though, because I want to think about it some more.

Long-term I want to simplify the rendering code, so that you don't need to implement a bunch of methods to introduce a feature like this one. I'd like to make it so that there's a "rendering payload" struct, similar to the RenderingPayload for AtlasEngine which simply describes what should be rendered and it's up to the rendering implementation to do with that information whatever it wants. So, for instance, instead of you calling into AtlasEngine to say "render these highlights now", you'd just fill the highlight vector on the RenderingPayload and then whenever AtlasEngine wants to it pulls out the relevant data. That way we skip an entire layer of code. The same would apply to cursors, selections, text colors, fonts, etc., etc.
I'm basically trying to see how I can make this PR fit with that. I won't ask you to modify your PR though. I'm just trying to map out what I need to do. 🙂

e82eric · 2023-11-03T17:44:49Z

src/renderer/atlas/common.h

@@ -377,6 +377,7 @@ namespace Microsoft::Console::Render::Atlas
    {
        u32 backgroundColor = 0;
        u32 selectionColor = 0x7fffffff;
+        u32 searchSelectionColor = 0x5fff0000;


I meant to ask about this, the blue I used probably isn't a great default, should I try to add this as setting?

Personally speaking, I wouldn't consider that important. Some of the most popular applications don't have configurable highlight colors, including Chromium and Word.

It looks like this value is no longer used. Should we remove it?

lhecker · 2023-11-03T18:03:36Z

While writing my previous comment I realized that Chromium changes the background and foreground color of text when marking it as highlighted:

It also doesn't seem to care about the current background color because picking the same color makes the highlight invisible:

Should we perhaps do the same? Get rid of _drawSearchSelections and just manipulate the background/foreground colors of the highlighted text (see second half of AtlasEngine::PaintBufferLine)?

Residential selection expert, @DHowett, what say thee? #5fff0000, aka rgba(255, 0, 0, 0.37) overlay or fixed #ff9632 background with #000000 foreground color, like Chromium?

@DHowett

Should we perhaps do the same? Get rid of _drawSearchSelections and just manipulate the background/foreground colors of the highlighted text (see second half of AtlasEngine::PaintBufferLine)? Residential selection expert, @DHowett, what say thee? #5fff0000, aka rgba(255, 0, 0, 0.37) overlay or fixed #ff9632 background with #000000 foreground color, like Chromium?

e82eric · 2023-11-04T07:18:51Z

While writing my previous comment I realized that Chromium changes the background and foreground color of text when marking it as highlighted:

It also doesn't seem to care about the current background color because picking the same color makes the highlight invisible:

Should we perhaps do the same? Get rid of _drawSearchSelections and just manipulate the background/foreground colors of the highlighted text (see second half of AtlasEngine::PaintBufferLine)?

Residential selection expert, @DHowett, what say thee? #5fff0000, aka rgba(255, 0, 0, 0.37) overlay or fixed #ff9632 background with #000000 foreground color, like Chromium?

Made an attempt at this, does the diff look like what you were thinking?

DHowett

I love this! I only have a few questions before I can sign off.

Thanks so much for bearing with us, and contributing a really awesome feature!

DHowett · 2023-11-28T02:24:16Z

src/renderer/atlas/common.h

@@ -377,6 +377,7 @@ namespace Microsoft::Console::Render::Atlas
    {
        u32 backgroundColor = 0;
        u32 selectionColor = 0x7fffffff;
+        u32 searchSelectionColor = 0x5fff0000;


It looks like this value is no longer used. Should we remove it?

DHowett · 2023-11-28T02:26:35Z

src/host/renderData.cpp

+// - Vector of Viewports describing the area selected
+std::vector<Viewport> RenderData::GetSearchSelectionRects() noexcept
+{
+    return {};


Conhost performance question: it's cheap to return an empty vector, right?

Gut check... it looks like it is "cheap enough", at least on x64:

#include <vector> std::vector<int> foo() { return {}; }

__$ReturnUdt$ = 32 std::vector<int,std::allocator<int> > foo(void) PROC ; foo, COMDAT $LN13: sub rsp, 24 xor eax, eax mov QWORD PTR [rcx], rax mov QWORD PTR [rcx+8], rax mov QWORD PTR [rcx+16], rax mov rax, rcx add rsp, 24 ret 0 std::vector<int,std::allocator<int> > foo(void) ENDP ; foo

DHowett · 2023-11-28T02:27:22Z

src/cascadia/TerminalCore/terminalrenderdata.cpp

@@ -188,6 +206,23 @@ void Terminal::SelectNewRegion(const til::point coordStart, const til::point coo
    SetSelectionEnd(realCoordEnd, SelectionExpansion::Char);
 }

+void Terminal::SelectSearchRegions(std::vector<til::inclusive_rect> rects)
+{


@lhecker do we need to use any of your locks here?

I'd say no. I moved all locking calls to outside of the Terminal class to make the code more consistent. If you were talking about the _assertLocked(), then I'd also say no, because this will be indirectly covered through the _VisibleStartIndex (we could add _assertLocked() to all member functions in the future if we wanted to).

DHowett · 2023-11-28T02:32:16Z

src/cascadia/TerminalCore/terminalrenderdata.cpp

+        auto rr = til::inclusive_rect{ realStart.x, realStart.y, realEnd.x, realEnd.y };
+
+        _searchSelections.emplace_back(rr);
+    }


I was expecting to find something here poking the renderer.. like TriggerSelection. Is that already happening because we're also making a new selection when we search?

I am worried about the hidden coupling here.

I think right now it is relying on this call to trigger selection.

terminal/src/cascadia/TerminalControl/ControlCore.cpp

Line 1624 in 9b823b0

_renderer->TriggerSelection();

Something does feel weird when I compare it to have the normal selection from _search.SelectCurrent() works.

Would it make sense to move the _searcher.HighlightSelections() here?

terminal/src/cascadia/TerminalControl/ControlCore.cpp

Line 1622 in 9b823b0

// We don't want to show the markers so manually tell it to clear it.

Or to add a TriggerSelection call here?

terminal/src/cascadia/TerminalControl/ControlCore.cpp

Line 1608 in 9b823b0

_searcher.MoveToCurrentSelection();

That's a really good question. I think that you'll want to do it any time you are done changing _searchSelections. @lhecker might have a better opinion here.

I tried out your branch! It works really, really well. Congrats!

I only found one issue.

When I delete the search with backspace, or close it, the highlights stick around... but the search is no longer active. So next time the screen updates, they all go away.

I think this might be because of the missing TriggerSelection!

somewhat long story on the selections not getting cleared when the last char is deleted from the search box. I was trying to match the behavior from the release that I had on my machine and it looked like the "current" selection highlight stuck around after deleting the last char and then I cross referenced visual studio and it looked like it also left the highlights so I thought it might be a pattern and didn't want to change any existing behavior.

For clearing all selections, I think this would work.
e82eric@4dc6305

For leaving the current selection highlighted but clearing all of the other highlights, I think this would work.
e82eric@3854b8b

For clearing the selections after the search box is closed, I actually like the behavior where it leaves the selections highlighted, It gives me context if I want to go into mark mode (about what is going to get selected).

I think it is possible to clear the selections after closing the search box.
e82eric@8a61353

Let me know if any of these make sense or if there is another approach and I can update the PR.

That all makes sense!

should be cleared.

lhecker

I don't see any obvious flaws personally. I think we should merge this now and fix any issues that arise later (ourselves) once we test this.

DHowett · 2023-12-15T21:01:56Z

@e82eric thanks so much for doing this! Pretty brave of you to jump into this area and just start making a difference. 😄

zadjii-msft · 2024-01-26T16:00:49Z

Hey I just want to chime in here: I've been selfhosting canary builds with this PR for a while now, and I have found this INFINITELY more helpful than I thought it would be. Thanks again @e82eric!

e82eric · 2024-01-27T01:09:25Z

No problem!

… wraps to next line (#16691) Hi, I realized I had a bug in my pull request for search selections where when the highlight is wrapped it causes the selection to select back to start of the current line instead of wrapping to the next line. PR where I introduced this bug: #16227

… wraps to next line (#16691) Hi, I realized I had a bug in my pull request for search selections where when the highlight is wrapped it causes the selection to select back to start of the current line instead of wrapping to the next line. PR where I introduced this bug: #16227 (cherry picked from commit bef2340) Service-Card-Id: 91808626 Service-Version: 1.20

…6227)" This reverts commit 28acc10.

This wasn't guarded under velocity, and some of the fixes are coming in too hot to be serviced into 1.20. Reverts PR #16227 and #16691

antshar · 2024-05-18T15:41:37Z

Why was it reverted? Can I have this feature on 1.20+?

DHowett · 2024-05-19T20:37:09Z

Why was it reverted?

It was not ready to be released to a hundred million people via the stable version.

Can I have this feature on 1.20+?

Yes, but only the “+” part of “1.20+”

gamepad-coder · 2024-06-30T22:24:48Z

Because this was reverted,
should issue #7561 be marked as open again?

Or is this PR still complete as-is and just waiting to be re-released?

This comment has been minimized.

Sign in to view

e82eric force-pushed the main branch 4 times, most recently from 577bcc0 to d8c0771 Compare October 25, 2023 15:36

Update to highlight all search results

8a5dfc6

microsoft#7561 Follows the existing selection code as much as possible.

e82eric force-pushed the main branch from d8c0771 to 8a5dfc6 Compare October 25, 2023 15:45

lhecker reviewed Nov 1, 2023

View reviewed changes

e82eric added 10 commits November 1, 2023 10:10

WIP

bf881d3

y < _p.rows.size() will always be true due to the preceding clamp() on y. A better way to express this would be to put this at the start of the loop: if (rect.bottom <= 0 || rect.top >= _p.s->viewportCellCount.y) { continue; }

WIP

ceb740f

I don't think this is right. This returns the rectangles manually selected by the user and not the rectangles selected during a search. This should return nothing.

WIP

d8df3d9

Fix for issue where search result is not highlighted when it starts on the first character of the line.

Removed accidental white space

7753648

WIP

2947771

check search rectangles before calling PaintSelections

WIP

0cef34c

Removing more accidental white space

WIP

328f809

Fixed from Invoke-CodeFormat

e82eric commented Nov 3, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

WIP cleaned up some left over code

8a0cbdd

This comment has been minimized.

Sign in to view

Fixed spelling mistakes

e5b412b

lhecker mentioned this pull request Nov 7, 2023

Remove DxEngine #16278

Merged

microsoft-github-policy-service bot requested review from DHowett and lhecker November 27, 2023 13:42

DHowett requested changes Nov 28, 2023

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Second It's a PR that needs another sign-off labels Nov 28, 2023

Removed unused searchSelectionsColor memeber

921e773

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 28, 2023

zadjii-msft assigned DHowett Dec 5, 2023

WIP: When the last char of the search box is removed all selections

4dc6305

should be cleared.

This comment has been minimized.

Sign in to view

e82eric requested a review from DHowett December 15, 2023 00:56

Merge remote-tracking branch 'origin/main' into e82eric/main

b6b362a

lhecker approved these changes Dec 15, 2023

View reviewed changes

DHowett approved these changes Dec 15, 2023

View reviewed changes

DHowett changed the title ~~Update to highlight all search results~~ Highlight all search results while the search box is open Dec 15, 2023

DHowett enabled auto-merge (squash) December 15, 2023 21:04

DHowett merged commit 28acc10 into microsoft:main Dec 15, 2023
13 of 15 checks passed

e82eric mentioned this pull request Feb 9, 2024

Fix for search selections highlight going backward when the selection wraps to next line #16691

Merged

4 tasks

DHowett added a commit that referenced this pull request Apr 22, 2024

Revert "Highlight all search results while the search box is open (#1…

780d8a1

…6227)" This reverts commit 28acc10.

DHowett mentioned this pull request Apr 22, 2024

[1.20] Revert search result highlighting from stable #17103

Merged

DHowett added a commit that referenced this pull request Apr 23, 2024

[1.20] Revert search result highlighting from stable (#17103)

7637e89

This wasn't guarded under velocity, and some of the fixes are coming in too hot to be serviced into 1.20. Reverts PR #16227 and #16691

microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight all search results while the search box is open #16227

Highlight all search results while the search box is open #16227

e82eric commented Oct 25, 2023 •

edited by lhecker

Loading

e82eric commented Oct 25, 2023

This comment has been minimized.

lhecker left a comment

e82eric commented Nov 2, 2023

lhecker commented Nov 3, 2023

e82eric Nov 3, 2023

lhecker Nov 3, 2023

DHowett Nov 28, 2023

lhecker commented Nov 3, 2023

This comment has been minimized.

This comment has been minimized.

e82eric commented Nov 4, 2023

DHowett left a comment

DHowett Nov 28, 2023

DHowett Nov 28, 2023

DHowett Nov 28, 2023

lhecker Dec 6, 2023

DHowett Nov 28, 2023

e82eric Nov 28, 2023

DHowett Dec 5, 2023

e82eric Dec 6, 2023 •

edited

Loading

DHowett Dec 15, 2023

This comment has been minimized.

lhecker left a comment

DHowett commented Dec 15, 2023

zadjii-msft commented Jan 26, 2024

e82eric commented Jan 27, 2024

antshar commented May 18, 2024

DHowett commented May 19, 2024 •

edited

Loading

gamepad-coder commented Jun 30, 2024

Highlight all search results while the search box is open #16227

Highlight all search results while the search box is open #16227

Conversation

e82eric commented Oct 25, 2023 • edited by lhecker Loading

Summary of the Pull Request

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

e82eric commented Oct 25, 2023

This comment has been minimized.

lhecker left a comment

Choose a reason for hiding this comment

e82eric commented Nov 2, 2023

lhecker commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Nov 3, 2023

This comment has been minimized.

This comment has been minimized.

e82eric commented Nov 4, 2023

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e82eric Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

lhecker left a comment

Choose a reason for hiding this comment

DHowett commented Dec 15, 2023

zadjii-msft commented Jan 26, 2024

e82eric commented Jan 27, 2024

antshar commented May 18, 2024

DHowett commented May 19, 2024 • edited Loading

gamepad-coder commented Jun 30, 2024

e82eric commented Oct 25, 2023 •

edited by lhecker

Loading

e82eric Dec 6, 2023 •

edited

Loading

DHowett commented May 19, 2024 •

edited

Loading