Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+82 −17
Open

Add support for tabbing to embedded hyperlinks #18347

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b50ddb1
Add support for tabbing to embedded hyperlinks
carlos-zamora Dec 21, 2024
51b7124
unindent
carlos-zamora Feb 4, 2025
aa612b3
apply feedback; fix crash when selection off end
carlos-zamora Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 58 additions & 15 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,8 @@ void Terminal::SelectHyperlink(const SearchDirection dir)
}

// 0. Useful tools/vars
const auto bufferSize = _activeBuffer().GetSize();
const auto& buffer = _activeBuffer();
const auto bufferSize = buffer.GetSize();
const auto viewportHeight = _GetMutableViewport().Height();

// The patterns are stored relative to the "search area". Initially, this search area will be the viewport,
Expand Down Expand Up @@ -547,27 +548,69 @@ void Terminal::SelectHyperlink(const SearchDirection dir)
searchArea = Viewport::FromDimensions(searchStart, { searchEnd.x + 1, searchEnd.y + 1 });
}
}
}

// 1.C) Nothing was found. Bail!
if (!result.has_value())
// 2. We found a hyperlink from the pattern tree. Look for embedded hyperlinks too!
// Use the result (if one was found) to narrow down the search.
searchStart = dir == SearchDirection::Forward ?
_selection->start :
(result ? result->second : bufferSize.Origin());
searchEnd = dir == SearchDirection::Forward ?
(result ? result->first : buffer.GetLastNonSpaceCharacter()) :
_selection->start;
Copy link
Member

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?

Copy link
Member Author

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

auto iter = buffer.GetCellDataAt(dir == SearchDirection::Forward ? searchStart : searchEnd);
while (dir == SearchDirection::Forward ? iter.Pos() < searchEnd : iter.Pos() > searchStart)
{
// Don't let us select the same hyperlink again
if (iter.Pos() < _selection->start || iter.Pos() > _selection->end)
{
return;
if (auto attr = iter->TextAttr(); attr.IsHyperlink())
Copy link
Member

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?

Copy link
Member Author

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.

{
// Found an embedded hyperlink!
const auto hyperlinkId = attr.GetHyperlinkId();

// Expand the start to include the entire hyperlink
TextBufferCellIterator hyperlinkStartIter{ buffer, iter.Pos() };
while (attr.IsHyperlink() && attr.GetHyperlinkId() == hyperlinkId)
{
hyperlinkStartIter--;
attr = hyperlinkStartIter->TextAttr();
}
// undo a move to be inclusive
hyperlinkStartIter++;

// Expand the end to include the entire hyperlink
// No need to undo a move! We'll decrement in the next step anyways.
TextBufferCellIterator hyperlinkEndIter{ buffer, iter.Pos() };
attr = hyperlinkEndIter->TextAttr();
while (attr.IsHyperlink() && attr.GetHyperlinkId() == hyperlinkId)
{
hyperlinkEndIter++;
attr = hyperlinkEndIter->TextAttr();
}

result = { hyperlinkStartIter.Pos(), hyperlinkEndIter.Pos() };
break;
}
}
iter += dir == SearchDirection::Forward ? 1 : -1;
}

// 2. Select the hyperlink
// 3. Select the hyperlink, if one exists
if (!result.has_value())
{
auto selection{ _selection.write() };
wil::hide_name _selection;
selection->start = result->first;
selection->pivot = result->first;
selection->end = result->second;
_selectionIsTargetingUrl = true;
_selectionEndpoint = SelectionEndpoint::End;
return;
}

// 3. Scroll to the selected area (if necessary)
_ScrollToPoint(_selection->end);
auto selection{ _selection.write() };
wil::hide_name _selection;
selection->start = result->first;
selection->pivot = result->first;
selection->end = result->second;
_selectionIsTargetingUrl = true;
_selectionEndpoint = SelectionEndpoint::End;

// 4. Scroll to the selected area (if necessary)
_ScrollToPoint(selection->end);
}

Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey) const noexcept
Expand Down
Loading