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

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.

oss/interval_tree/MAINTAINER_README.md

oss/interval_tree/LICENSE

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

ghost · 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.

… URL detecting (#8124) Fix #8121 ![image](https://user-images.githubusercontent.com/1068203/97811235-2081ca80-1cb4-11eb-82bd-1ddaf15c757c.png)  ## Summary of the Pull Request When calculating the position of the matched pattern, consider the width of the characters. However, if there are some wide glyphs in the detected hyperlink(not possible for now, for the existing regex will not match wide-character?). The repeated character in the tooltip is not fixed by this PR.  ## References  ## PR Checklist * [ ] Closes #8121 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments When calculating the coordinate of the match in #7691, it simply uses the `prefix.size()` as the total prefix width on the screen. This PR fixes that behavior.  ## Validation Steps Performed Manually Verified

ghost · 2020-11-11T18:14:53Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-11-15T04:36:05Z

Scott's wish finally became true: https://youtu.be/veqs2WVou9M?t=797

xeho91 · 2020-11-15T14:22:38Z

I love this feature!

I have a question, would it be possible to make this hovering on the link customizeable? I know it gives an underline effects, but in the scenario where I have all of the links already "underlined", I can't see if I am hovering on a link or not.

The example of custom customization would be to give a background color for the text with link or something similar.

ghost · 2020-11-15T14:42:21Z

I guess that would be another issue

DHowett · 2020-11-15T18:46:13Z

@xeho91 today it underlines on hover. Is that not working for you?

xeho91 · 2020-11-15T19:11:02Z

@xeho91 today it underlines on hover. Is that not working for you?

It does. However, in the example when I am using bat to preview my *.md files. It has a syntax which already recognizes and underlines the links in the output. There's a quick preview below.

When I am hovering with the cursor, there's no "visual feedback", except this small dialogue box "Ctrl + Click to follow link.".

What I am asking for to give us ability to define the style effect in settings for hovering on links. Like make the background white, and black text. If that's possible, no pressure, I still love this feature. ❤️

zadjii-msft · 2020-11-16T03:40:54Z

@xeho91 I've tossed this request into #5001, since this isn't actually the first time this has come up 😉 Thanks!

joshuabiddle · 2023-03-03T20:18:30Z

I'd like to use this ability to open an html file stored on a remote computer using the UNC path. I haven't been able to get it to open. Any ideas?

Example: \COMPUTER\UNC\Path\To\File.html

rough implementation

481c16e

ghost added Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 21, 2020

PankajBhojwani added 3 commits September 21, 2020 13:07

spell/format

062e780

const/c cast fix

973ed97

initialize inc

dbb9a9d

This was referenced Sep 22, 2020

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

Closed

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

Closed

PankajBhojwani added 2 commits September 25, 2020 11:37

prototype

e4e5858

const

4db85a4