Add support for tabbing to embedded hyperlinks by carlos-zamora · Pull Request #18347 · microsoft/terminal

carlos-zamora · 2024-12-21T00:19:09Z

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

src/cascadia/TerminalCore/TerminalSelection.cpp

DHowett · 2025-01-21T23:12:52Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+        if (iter.Pos() < _selection->start || iter.Pos() > _selection->end)
        {
-            return;
+            if (auto attr = iter->TextAttr(); attr.IsHyperlink())


qq: do we have a way to iterate by attribute instead of by cell?

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.

DHowett

(blocking so i see it later in my list of PRs)

lhecker

Just a minor nits about iterators, and a question about the loop boundaries!

src/cascadia/TerminalCore/TerminalSelection.cpp

zadjii-msft · 2025-02-06T22:12:46Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+    searchStart = dir == SearchDirection::Forward ?
+                      _selection->start :
+                      (result ? result->second : bufferSize.Origin());
+    searchEnd = dir == SearchDirection::Forward ?
+                    (result ? result->first : buffer.GetLastNonSpaceCharacter()) :
+                    _selection->start;


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?

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: ```powershell ${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 Closes #15194 Follow-up from #13405 OSC 8 support added in #7251 (cherry picked from commit 35bd607) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXBDb8 Service-Version: 1.23

carlos-zamora added the zBugBash-Consider label Jan 14, 2025

carlos-zamora added a commit that referenced this pull request Jan 20, 2025

PRE-MERGE #18347 Add support for tabbing to embedded hyperlinks

44f19bd

DHowett requested a review from lhecker January 21, 2025 23:10

DHowett reviewed Jan 21, 2025

View reviewed changes

DHowett requested changes Jan 24, 2025

View reviewed changes

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 bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jan 31, 2025

Add support for tabbing to embedded hyperlinks

b50ddb1

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

unindent

51b7124

carlos-zamora force-pushed the dev/cazamor/tab-to-embedded-hyperlink branch from d27cba9 to 51b7124 Compare February 4, 2025 22:26

DHowett approved these changes Feb 5, 2025

View reviewed changes

lhecker reviewed Feb 6, 2025

View reviewed changes

zadjii-msft reviewed Feb 6, 2025

View reviewed changes

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) labels Feb 19, 2025

apply feedback; fix crash when selection off end

aa612b3

carlos-zamora assigned lhecker and zadjii-msft Feb 24, 2025

lhecker added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Feb 24, 2025

lhecker approved these changes Feb 24, 2025

View reviewed changes

carlos-zamora unassigned lhecker and zadjii-msft Feb 24, 2025

carlos-zamora merged commit 35bd607 into main Feb 24, 2025
19 checks passed

carlos-zamora deleted the dev/cazamor/tab-to-embedded-hyperlink branch February 24, 2025 23:55

DHowett moved this from To Cherry Pick to Cherry Picked in 1.23 Servicing Pipeline Feb 26, 2025

microsoft deleted a comment from jourshmurs Apr 14, 2025

DHowett moved this from Cherry Picked to Shipped in 1.23 Servicing Pipeline Sep 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tabbing to embedded hyperlinks#18347

Add support for tabbing to embedded hyperlinks#18347
carlos-zamora merged 3 commits intomainfrom
dev/cazamor/tab-to-embedded-hyperlink

carlos-zamora commented Dec 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

DHowett Jan 21, 2025

Uh oh!

carlos-zamora Feb 4, 2025

Uh oh!

DHowett left a comment

Uh oh!

lhecker left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft Feb 6, 2025

Uh oh!

carlos-zamora Feb 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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);
	}
	}
	}

Conversation

carlos-zamora commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

Validation Steps Performed

Uh oh!

Uh oh!

DHowett Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Dec 21, 2024 •

edited

Loading