Add a setting to disable URL detection #10022

PankajBhojwani · 2021-05-03T14:03:08Z

Summary of the Pull Request

Adds a global setting, experimental.detectHyperlinks, that controls whether we automatically detect links and make them clickable. Default is set to true.

PR Checklist

Closes Disable "follow link" #9981
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I work here

Validation Steps Performed

When detectHyperlinks is set to false, links do not underline on hover and are not clickable.

PankajBhojwani · 2021-05-03T14:05:28Z

Note, we do not update the terminal's behaviour upon hot reload of this setting, changes to it will only apply to future terminal instances. If people feel strongly about this we could change that but I don't think a hot reload for this is necessary.

EDIT: We update the behaviour with regards to this setting on hot reload now

carlos-zamora · 2021-05-03T23:27:19Z

Note, we do not update the terminal's behaviour upon hot reload of this setting, changes to it will only apply to future terminal instances. If people feel strongly about this we could change that but I don't think a hot reload for this is necessary.

I hate to say it, but I think this should be hot reloadable :/. Here's a few reasons:

scenario:
- I'm a random user using the terminal. I see that hyperlinks are on and I think "let me turn it off". I go to the settings and turn it off. But when I go back to the terminal, it's still there. So now I file a bug on the repo saying that it doesn't work.
consistency:
- It looks like all of the other interactivity global settings are hot reload-able, so it's a bit confusing for this one not to be.
technical reason:
- We at the very least should update the Settings UI and schema to say "requires restart" to make this clear to the user.
- Assuming we decide to make this hot reloadable in the future, then we need to do a whole dance with the localization where one key says "requires restart" but the other one doesn't. And we'll need to keep the localization key around forever even though we don't care about the old one anymore. (this one is just a pet peeve, really)

carlos-zamora · 2021-05-05T17:16:00Z

Also, what's the deal with #8293? Specifically thinking about this comment with regards to the future of url detection customization.

zadjii-msft · 2021-05-06T12:37:08Z

Also, what's the deal with #8293? Specifically thinking about this comment with regards to the future of url detection customization.

That thread is the entire explanation of why we didn't add this in the first place. We couldn't agree on a simple setting name/design in the first place. Since we couldn't come to a consensus, and no one was asking for the setting, we decided to punt the discussion.

I'm tempted to say: let's toss this in as experimental.detectHyperlinks, so that we can yank it in the future, when we have a real design. That way we can ship this now, without having to fully explore whatever we want from pattern detection. If we want to end up keeping it, we can. If we want to fully replace it with profile:detectPatterns or profile:patterns or an extension or whatever in the future, we can yank it then.

PankajBhojwani · 2021-05-06T15:04:26Z

I like the experimental idea, made that change. Also, the link detection behaviour gets updated with hot reloads now.

carlos-zamora

Just a minor suggestion :)

src/buffer/out/textBuffer.cpp

carlos-zamora · 2021-05-06T17:10:39Z

src/cascadia/TerminalControl/ControlCore.cpp

-        _terminal->UpdatePatterns();
+        auto lock = _terminal->LockForWriting();
+        _terminal->UpdatePatternsUnderLock();


Just curious, why'd you split this up?

Calling UpdatePatterns from UpdateSettings was causing a hang because of the writing lock, and I didn't want to have both an UpdatePatterns and an UpdatePatternsUnderLock. And since this is the only other place we ask Terminal to update the pattern locations, I thought it'd be fine to ask it to grab the lock

DHowett

Blocking to note that we discussed a way to clean up the hot reload logic.

Every time there's a settings reload, we can simply always clear pattern recognizers and re-register the link pattern. We can probably also avoid the detection of the existing pattern recognizer.

Clearing the pattern recognizers: we could reset _currentPatternId to 0 again? I dunno if we need it.

PankajBhojwani · 2021-05-12T05:15:22Z

Every time there's a settings reload, we can simply always clear pattern recognizers and re-register the link pattern.

Made the change!

Clearing the pattern recognizers: we could reset _currentPatternId to 0 again? I dunno if we need it.

I like that just for neatness

DHowett

It's more correct to say that this controls detecting URLs. Hyperlinks are the things we're making out of the URLs, right? Should we reword the JSON key and the settings UI checkbox?

PankajBhojwani · 2021-05-17T03:58:02Z

It's more correct to say that this controls detecting URLs. Hyperlinks are the things we're making out of the URLs, right? Should we reword the JSON key and the settings UI checkbox?

Makes sense! Renamed to detectURLs everywhere for consistency

ghost · 2021-05-17T04:05:09Z

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.

ghost · 2021-05-25T19:09:54Z

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

Handy links:

setting to disable link detection

66a9faf

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 3, 2021

hot reload 1

caa88d6

PankajBhojwani added 5 commits May 6, 2021 07:05

experimental, hot reload

f1c68c2

undo hot reload for now, requires more thought than I realized

b8f3f87

don't need clear without hot reload

407171b

ok no not that bad

010a824

conflict

dbc138d

carlos-zamora approved these changes May 6, 2021

View reviewed changes

unpack

dcbc767

DHowett requested changes May 11, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 11, 2021

clear and reload

e235679

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 12, 2021

zadjii-msft approved these changes May 13, 2021

View reviewed changes

zadjii-msft assigned DHowett May 13, 2021

DHowett approved these changes May 17, 2021

View reviewed changes

rename

3ff2b97

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 17, 2021

ghost merged commit 7a41be5 into main May 17, 2021

ghost deleted the dev/pabhoj/disable_link branch May 17, 2021 04:20

M0nk3yH4cks mentioned this pull request May 17, 2021

[1.9] Crash opening Settings UI #10106

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a setting to disable URL detection #10022

Add a setting to disable URL detection #10022

PankajBhojwani commented May 3, 2021 •

edited

Loading

PankajBhojwani commented May 3, 2021 •

edited

Loading

carlos-zamora commented May 3, 2021

carlos-zamora commented May 5, 2021

zadjii-msft commented May 6, 2021

PankajBhojwani commented May 6, 2021

carlos-zamora left a comment

carlos-zamora May 6, 2021

PankajBhojwani May 7, 2021

DHowett left a comment

PankajBhojwani commented May 12, 2021 •

edited

Loading

DHowett left a comment

PankajBhojwani commented May 17, 2021

ghost commented May 17, 2021

ghost commented May 25, 2021

Add a setting to disable URL detection #10022

Add a setting to disable URL detection #10022

Conversation

PankajBhojwani commented May 3, 2021 • edited Loading

Summary of the Pull Request

PR Checklist

Validation Steps Performed

PankajBhojwani commented May 3, 2021 • edited Loading

carlos-zamora commented May 3, 2021

carlos-zamora commented May 5, 2021

zadjii-msft commented May 6, 2021

PankajBhojwani commented May 6, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora May 6, 2021

Choose a reason for hiding this comment

PankajBhojwani May 7, 2021

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

PankajBhojwani commented May 12, 2021 • edited Loading

DHowett left a comment

Choose a reason for hiding this comment

PankajBhojwani commented May 17, 2021

ghost commented May 17, 2021

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.

ghost commented May 25, 2021

PankajBhojwani commented May 3, 2021 •

edited

Loading

PankajBhojwani commented May 3, 2021 •

edited

Loading

PankajBhojwani commented May 12, 2021 •

edited

Loading

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.