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.

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

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Dec 21, 2024

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:

${ESC}=[char]27
Write-Host "${ESC}]8;;https://github.com/microsoft/terminal${ESC}\This is a link!${ESC}]8;;${ESC}\"

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
Follow-up from #13405
OSC 8 support added in #7251

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Product-Terminal The new Windows Terminal. labels Dec 21, 2024
@DHowett DHowett requested a review from lhecker January 21, 2025 23:10
src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
{
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:

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.

Copy link
Member

@DHowett DHowett left a 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)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 24, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jan 31, 2025
@carlos-zamora carlos-zamora removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Feb 4, 2025
@microsoft-github-policy-service 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 Feb 4, 2025
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/tab-to-embedded-hyperlink branch from d27cba9 to 51b7124 Compare February 4, 2025 22:26
Copy link
Member

@lhecker lhecker left a 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!

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

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 {
}

TextBufferCellIterator hyperlinkStartIter{ buffer, iter.Pos() };
while (attr.IsHyperlink() && attr.GetHyperlinkId() == hyperlinkId)
{
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.


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

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be able to support clicking on embedded links in the mark mode
4 participants