Add support for autodetecting URLs and making hyperlinks #7691

PankajBhojwani · 2020-09-21T17:05:24Z

This pull request is the initial implementation of hyperlink auto
detection

Overall design:

Upon startup, TerminalCore gives the TextBuffer some patterns it
should know about
Whenever something in the viewport changes (i.e. text
output/scrolling), TerminalControl tells TerminalCore (through a
throttled function for performance) to retrieve the visible pattern
locations from the TextBuffer
When the renderer encounters a region that is associated with a
pattern, it paints that region differently

References #5001
Closes #574

github-actions · 2020-09-21T17:00:23Z

New misspellings found, please review:

sregex

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"Autogenerated debugbreak DECLL DECSMBV Inplace notypeopt restrictederrorinfo Scs Switchto Wlk "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/481c16ea1ea1e413e6b3af8841983a375c86baad.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"autogenerated inplace sregex "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/481c16ea1ea1e413e6b3af8841983a375c86baad.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

codeofdusk · 2020-09-22T14:12:25Z

It doesn't look like this PR adds any custom logic to the UIA text range, so I doubt UIA clients will be able to detect clickable links with this implementation. Have you tested for this (i.e. with Narrator/NVDA)?

As an aside, this PR makes a good case for allowing browse mode in terminals (nvaccess/nvda#11172).
Cc @carlos-zamora, @Neurrone.

carlos-zamora · 2020-09-22T18:27:58Z

Regarding accessibility. I haven't had a chance to test this out yet. Especially since it's in draft. But here's some resources and an idea of the work we'll probably have to do (probably in a separate PR):

In UiaTextRange, we'll need to update...

GetChildren(): hyperlink ranges count as a child
GetEnclosingElement(): if we're on a hyperlink, return the parent text range. Otherwise, normal behavior.
FindAttribute() and GetAttribute(): hyperlinks need to have the hyperlink attribute (exposed via GetAttribute()). And FindAttribute() needs to be implemented to find if one exists in the text range.

Another thought. Hyperlink support is just for Windows Terminal, not ConHost, right? That'll complicate things a bit with our accessibility model because UiaTextRange is currently shared. I see the implementation for autodetection is in TextBuffer (which makes this easier), but if ConHost won't expose it, we need to figure out a way to not expose hyperlinks that don't exist in ConHost. Of course, if this functionality is shared, just completely ignore this paragraph haha.

codeofdusk · 2020-09-22T18:30:31Z

Hyperlink support is just for Windows Terminal, not ConHost, right?

If this is the plan, might this be reconsidered? While Windows Terminal is very feature-filled, in my experience conhost has much wider usage.

DHowett · 2020-09-22T18:40:58Z

@codeofdusk, @carlos-zamora Hyperlink support is Terminal-FIRST, not Terminal-ONLY. Sorry for the confusion.

Since we can move so much faster in Terminal it lets us figure out the interaction model a lot sooner than if we throw something together and hope that people jump on the Windows Insider builds and test our thing out.

Many, many changes that make it into Terminal will eventually flow back. This is especially true for anything that deals with accessibility, standards compliance and text rendering.

We'd like to figure out the interaction model for formatted text and browseable regions in Terminal before we bring anything like this back into conhost, absolutely.

src/buffer/out/textBuffer.cpp

src/buffer/out/textBuffer.hpp

src/cascadia/TerminalCore/Terminal.cpp

src/buffer/out/textBuffer.cpp

miniksa · 2020-09-25T18:29:55Z

src/buffer/out/textBuffer.cpp

+            const auto startRow = gsl::narrow<SHORT>(start / rowSize);
+            const auto startCol = gsl::narrow<SHORT>(start % rowSize);
+            const auto endRow = gsl::narrow<SHORT>(end / rowSize);
+            const auto endCol = gsl::narrow<SHORT>(end % rowSize);
+            const COORD startCoord{ startCol, startRow };
+            const COORD endCoord{ endCol, endRow };


I am pretty sure this level of math is also done in Find/Selection and it might be using TIL or one of the utilities/types classes to help with it. Consider deduplicating with those.

Hm I can't seem to find the similar math - those parts of the code seem to begin with coordinates already (buffer or viewport) whereas here, since we only search after concatenating all the text together, we need to calculate the coordinates from the concatenated string

miniksa · 2020-09-25T18:30:42Z

Now I don't want to go too crazypants here, but I'm thinking about future extensions to some degree.

I'm curious if you considered a design where the text buffer figures out the patterns on its own instead of being pumped to do that action by a thread from way outside itself, in this case from up in the Terminal* and friends.

I'm not saying this one is bad or wrong, but an alternate vision I have is that you would register an object with the TextBuffer that contained a pattern, region of interest, and a callback method/event inside of it. The TextBuffer would just... of its own accord as things are written in on the other methods (and maybe with its own internal, private thread or threadpool worker) grind through the region with the pattern and make callbacks as necessary.

Almost like you have an ITextScanner interface with an ITextMatchFound callback interface... which might lend itself toward easy adaptation into a COM/WinRT-based plug-in model for arbitrary buffer scanners in the future.

miniksa · 2020-09-25T18:34:42Z

Now I don't want to go too crazypants here, but I'm thinking about future extensions to some degree.

I'm curious if you considered a design where the text buffer figures out the patterns on its own instead of being pumped to do that action by a thread from way outside itself, in this case from up in the Terminal* and friends.

I'm not saying this one is bad or wrong, but an alternate vision I have is that you would register an object with the TextBuffer that contained a pattern, region of interest, and a callback method/event inside of it. The TextBuffer would just... of its own accord as things are written in on the other methods (and maybe with its own internal, private thread or threadpool worker) grind through the region with the pattern and make callbacks as necessary.

Almost like you have an ITextScanner interface with an ITextMatchFound callback interface... which might lend itself toward easy adaptation into a COM/WinRT-based plug-in model for arbitrary buffer scanners in the future.

As Dustin and I are musing about this... your ITextScanner could just also contain ITextEffect on it as an optional in lieu of a callback that is just handed straight into the renderer and now you've also managed to design rendering effect extensions at the same time...

miniksa · 2020-09-25T18:57:33Z

Now I don't want to go too crazypants here, but I'm thinking about future extensions to some degree.
I'm curious if you considered a design where the text buffer figures out the patterns on its own instead of being pumped to do that action by a thread from way outside itself, in this case from up in the Terminal* and friends.
I'm not saying this one is bad or wrong, but an alternate vision I have is that you would register an object with the TextBuffer that contained a pattern, region of interest, and a callback method/event inside of it. The TextBuffer would just... of its own accord as things are written in on the other methods (and maybe with its own internal, private thread or threadpool worker) grind through the region with the pattern and make callbacks as necessary.
Almost like you have an ITextScanner interface with an ITextMatchFound callback interface... which might lend itself toward easy adaptation into a COM/WinRT-based plug-in model for arbitrary buffer scanners in the future.

As Dustin and I are musing about this... your ITextScanner could just also contain ITextEffect on it as an optional in lieu of a callback that is just handed straight into the renderer and now you've also managed to design rendering effect extensions at the same time...

OK we all discussed this with Teams. I'm willing to concede this point because Hyperlinks are so interesting here that they push the boundaries of this sort of unified model. And where the threads live is a bit contentious. And Dustin keeps coming up with more ideas why, while this model sort of works, it has sharp edges. So I'm fine with proceeding per what this draft has as a design.

github-actions · 2020-09-28T19:09:13Z

New misspellings found, please review:

geeksforgeeks
wsregex

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"Autogenerated debugbreak DECLL DECSMBV Inplace notypeopt restrictederrorinfo Scs Switchto Wlk "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/8b95bd9719f5ee335555723cc5ac7fe0a218a53f.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"autogenerated geeksforgeeks inplace wsregex "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/8b95bd9719f5ee335555723cc5ac7fe0a218a53f.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

… before any scrolling happens

DHowett · 2020-10-23T21:03:01Z

SO! When I get a link at the top of the screen using the mouse wheel, and then use the scroll bar... something odd happens.

In general, scrollbar interactions seem to cause the URLs to break or get otherwise messed up.

PankajBhojwani · 2020-10-24T16:29:28Z

SO! When I get a link at the top of the screen using the mouse wheel, and then use the scroll bar... something odd happens.

Yeah we were only clearing/updating the pattern tree on mouse scrolling, changed it to update on either mouse or scroll bar scrolling

src/buffer/out/textBuffer.cpp

src/cascadia/TerminalCore/terminalrenderdata.cpp

src/cascadia/WindowsTerminalUniversal/WindowsTerminalUniversal.vcxproj

src/buffer/out/textBuffer.cpp

…/pabhoj/auto_link

src/buffer/out/textBuffer.hpp

src/renderer/base/renderer.cpp

src/cascadia/TerminalCore/Terminal.cpp

miniksa

Very close to signoff. Just a few oddball comments.

DHowett · 2020-10-28T17:57:45Z

I edited your PR body down to be a nice commit body :)

msftbot · 2020-10-28T20:24:29Z

Hello @PankajBhojwani!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

zadjii-msft · 2020-10-28T20:31:39Z

comzyh · 2020-10-31T10:02:11Z

I guess there is something wrong with handling wide characters.

rough implementation

Loading status checks…

481c16e

msftbot bot added Area-Extensibility Issue-Feature Product-Terminal labels Sep 21, 2020

Pankaj Bhojwani added 3 commits Sep 21, 2020

spell/format

Loading status checks…

062e780

const/c cast fix

Loading status checks…

973ed97

initialize inc

Loading status checks…

dbb9a9d

This was referenced Sep 22, 2020

Feature Idea: Find/Extract Text Attributes for Accessibility #2161

Open

Efficiently reviewing the output buffer in terminals nvaccess/nvda#11172

Open

Pankaj Bhojwani added 2 commits Sep 25, 2020

prototype

Loading status checks…

e4e5858

const

Loading status checks…

4db85a4