-
Notifications
You must be signed in to change notification settings - Fork 8.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Highlight all search results while the search box is open #16227
Changes from 11 commits
8a5dfc6
54e6f12
bf881d3
ceb740f
d8df3d9
4cbd3c1
7753648
2947771
8bce03b
0cef34c
328f809
c3eced3
8a0cbdd
e5b412b
ab374f4
2be6211
9b823b0
921e773
4dc6305
b6b362a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -150,6 +150,24 @@ catch (...) | |||||||
return {}; | ||||||||
} | ||||||||
|
||||||||
std::vector<Microsoft::Console::Types::Viewport> Terminal::GetSearchSelectionRects() noexcept | ||||||||
try | ||||||||
{ | ||||||||
std::vector<Viewport> result; | ||||||||
|
||||||||
for (const auto& lineRect : _GetSearchSelectionRects(_GetVisibleViewport())) | ||||||||
{ | ||||||||
result.emplace_back(Viewport::FromInclusive(lineRect)); | ||||||||
} | ||||||||
|
||||||||
return result; | ||||||||
} | ||||||||
catch (...) | ||||||||
{ | ||||||||
LOG_CAUGHT_EXCEPTION(); | ||||||||
return {}; | ||||||||
} | ||||||||
|
||||||||
void Terminal::SelectNewRegion(const til::point coordStart, const til::point coordEnd) | ||||||||
{ | ||||||||
#pragma warning(push) | ||||||||
|
@@ -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) | ||||||||
{ | ||||||||
_searchSelections.clear(); | ||||||||
for (auto& rect : rects) | ||||||||
{ | ||||||||
rect.top -= _VisibleStartIndex(); | ||||||||
rect.bottom -= _VisibleStartIndex(); | ||||||||
|
||||||||
const auto realStart = _ConvertToBufferCell(til::point{ rect.left, rect.top }); | ||||||||
const auto realEnd = _ConvertToBufferCell(til::point{ rect.right, rect.bottom }); | ||||||||
|
||||||||
auto rr = til::inclusive_rect{ realStart.x, realStart.y, realEnd.x, realEnd.y }; | ||||||||
|
||||||||
_searchSelections.emplace_back(rr); | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was expecting to find something here poking the renderer.. like I am worried about the hidden coupling here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think right now it is relying on this call to trigger selection.
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?
Or to add a TriggerSelection call here?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a really good question. I think that you'll want to do it any time you are done changing 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. For leaving the current selection highlighted but clearing all of the other highlights, I think this would work. 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. Let me know if any of these make sense or if there is another approach and I can update the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That all makes sense! |
||||||||
} | ||||||||
|
||||||||
const std::wstring_view Terminal::GetConsoleTitle() const noexcept | ||||||||
{ | ||||||||
_assertLocked(); | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,16 @@ std::vector<Viewport> RenderData::GetSelectionRects() noexcept | |
return result; | ||
} | ||
|
||
// Method Description: | ||
// - Retrieves one rectangle per line describing the area of the viewport | ||
// that should be highlighted in some way to represent a user-interactive selection | ||
// Return Value: | ||
// - Vector of Viewports describing the area selected | ||
std::vector<Viewport> RenderData::GetSearchSelectionRects() noexcept | ||
{ | ||
return {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
// Method Description: | ||
// - Lock the console for reading the contents of the buffer. Ensures that the | ||
// contents of the console won't be changed in the middle of a paint | ||
|
@@ -369,6 +379,10 @@ void RenderData::SelectNewRegion(const til::point coordStart, const til::point c | |
Selection::Instance().SelectNewRegion(coordStart, coordEnd); | ||
} | ||
|
||
void RenderData::SelectSearchRegions(std::vector<til::inclusive_rect> source) | ||
{ | ||
} | ||
|
||
// Routine Description: | ||
// - Gets the current selection anchor position | ||
// Arguments: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhecker do we need to use any of your locks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).