-
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
Add support for tabbing to embedded hyperlinks #18347
base: main
Are you sure you want to change the base?
Conversation
{ | ||
return; | ||
if (auto attr = iter->TextAttr(); attr.IsHyperlink()) |
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.
qq: do we have a way to iterate by attribute instead of by cell?
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.
Sorta, but it makes things more complicated and it's not really worth it imo.
We could use row.Attributes().runs()
kinda like TextBuffer::ClearMarksInRange()
here:
terminal/src/buffer/out/textBuffer.cpp
Lines 3462 to 3479 in 7423dd3
void TextBuffer::ClearMarksInRange( | |
const til::point start, | |
const til::point end) | |
{ | |
auto top = std::clamp(std::min(start.y, end.y), 0, _height - 1); | |
auto bottom = std::clamp(std::max(start.y, end.y), 0, _estimateOffsetOfLastCommittedRow()); | |
for (auto y = top; y <= bottom; y++) | |
{ | |
auto& row = GetMutableRowByOffset(y); | |
auto& runs = row.Attributes().runs(); | |
row.SetScrollbarData(std::nullopt); | |
for (auto& [attr, length] : runs) | |
{ | |
attr.SetMarkAttributes(MarkKind::None); | |
} | |
} | |
} |
So we'd be able to get the length of each attribute run, but the combination of search direction (searching backwards with this system is hard) and support for wrapped hyperlinks makes this more complicated than it's worth, imo.
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.
(blocking so i see it later in my list of PRs)
d27cba9
to
51b7124
Compare
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.
Just a minor nits about iterators, and a question about the loop boundaries!
searchStart = dir == SearchDirection::Forward ? | ||
_selection->start : | ||
(result ? result->second : bufferSize.Origin()); | ||
searchEnd = dir == SearchDirection::Forward ? | ||
(result ? result->first : buffer.GetLastNonSpaceCharacter()) : | ||
_selection->start; |
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.
wait so if we find a hyperlink we marked up because it looked like a URL regex, we'll then look inside the autodetected one for a manually delimited one?
am I reading that right?
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.
Applied Leonard's suggestion to help make searchStart/End more clear.
So, in step 1, the search space spans from our selection to the visible start/end index. We're able to extract a regex match from _patternIntervalTree
using findContained()
. Since we have to pass the search space into that function, we're just calling it over and over again with an expanded search space until we find something.
Step 2 has to use a TextBufferCellIterator
to search through the buffer for a hyperlink attribute. Assuming we already found a result
in step one, we can reduce the search space to be from our selection to the result
. (if no result was found, search up to the buffer boundaries*)
*additional optimization: use GetLastNonSpaceCharacter()
as end instead of bufferSize.End()
Summary of the Pull Request
There's already logic to tab to a hyperlink when we're in mark mode. We do this by looking at the automatically detected hyperlinks and finding the next one of interest. This adds an extra step afterwards to find any embedded hyperlinks and tab to them too.
Since embedded hyperlinks are stored as text attributes, we need to iterate through the buffer to find the hyperlink and it's buffer boundaries. This PR tries to reduce the workload of that by first finding the automatically detected hyperlinks (since that's a fairly quick process), then using the reduced search area to find the embedded hyperlink (if one exists).
Validation Steps Performed
In PowerShell, add an embedded hyperlink as such:
Enter mark mode (ctrl+shift+m) then shift+tab to it.
✅ The "This is a link!" is selected
✅ Verified that this works when searching forwards and backwards
Closes #18310
Closes #15194
Follow-up from #13405
OSC 8 support added in #7251