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 2 commits into
base: main
Choose a base branch
from
Open

Add support for tabbing to embedded hyperlinks #18347

Changes from all commits
Commits
Show all changes
2 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
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;
Comment on lines +555 to +560
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of obtuse IMO, but even after thinking about it for a while I can't think of a better way to write this. One option may to explicitly split it up into

if (dir == SearchDirection::Forward) {
} else {
}

Comment on lines +555 to +560
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?

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the hyperlink starts at the origin of the search, couldn't this accidentally leave the boundaries of it?

{
hyperlinkStartIter--;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always write --hyperlinkStartIter if you don't want the previous result.

attr = hyperlinkStartIter->TextAttr();
}
// undo a move to be inclusive
hyperlinkStartIter++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here and just below.


// 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