🐛 Windows Terminal Preview 1.4: Hyperlinks not working

[daniel-liuzzi]

So I was trying the Hyperlink support in Windows Terminal Preview 1.4 but it doesn't seem to work (hyperlinks source is shown instead of actual links.)

Raw output

❯ git --no-pager show dd88342018aa239c77d7667bc988f0ff5db4610a
commit dd88342018aa239c77d7667bc988f0ff5db4610a
Author: Dan Davison <dandavison7@gmail.com>
Date:   Thu Sep 17 21:35:46 2020 -0400

    cargo fmt

diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs
index ba847c2..9f1a257 100644
--- a/src/tests/test_example_diffs.rs
+++ b/src/tests/test_example_diffs.rs
@@ -819,11 +819,15 @@ src/align.rs
             "--line-numbers",
         ]);
         let output = integration_test_utils::run_delta(GIT_DIFF_SINGLE_HUNK, &config);
-        assert!(output.contains("
-@@ -71,11 +71,8 @@ impl<'a> Alignment<'a> {"));
-        assert!(!output.contains("
+        assert!(output.contains(
+            "
+@@ -71,11 +71,8 @@ impl<'a> Alignment<'a> {"
+        ));
+        assert!(!output.contains(
+            "

-@@ -71,11 +71,8 @@ impl<'a> Alignment<'a> {"));
+@@ -71,11 +71,8 @@ impl<'a> Alignment<'a> {"
+        ));
         ansi_test_utils::assert_line_has_no_color(
             &output,
             9,

Delta output

Other information

If I instead try the "Validation Steps Performed" snippet in microsoft/terminal#7251 (comment), it works as expected:

In case it helps, these are the relevant options in my .gitconfig:

[core]
	pager = delta

[delta]
	hyperlinks = true
	line-numbers = true
	line-numbers-minus-style = "#660000"
	line-numbers-zero-style = "#333333"
	line-numbers-plus-style = "#006600"
	minus-style = "#999999" "#330000"
	plus-style = "#999999" "#003300"
	minus-emph-style = "#cccccc" "#660000"
	plus-emph-style = "#cccccc" "#006600"
	syntax-theme = TwoDark

I also added this to my Microsoft.PowerShell_profile.ps1 (as per as per microsoft/terminal#7251)

$ESC = "`e"

[dandavison]

Hi @daniel-liuzzi, this is cool, I didn't know Windows Terminal was starting to support hyperlinks. Let's try to work out what's going wrong.

I think what I'd do first is capture the raw output of delta with hyperlinks. For example, piping git show into delta and then into bat -A gives me

delta(master) git show dd88342018aa239c77d7667bc988f0ff5db4610a | delta | bat --plain -A
commit·dd88342018aa239c77d7667bc988f0ff5db4610a␊
Author:·Dan·Davison·<dandavison7@gmail.com>␊
Date:···Thu·Sep·17·21:35:46·2020·-0400␊
␊
····cargo·fmt␊
␊
␊
␛[38;5;4m␛]8;;file:///Users/dan/src/delta/src/tests/test_example_diffs.rs␛\src/tests/test_example_diffs.rs␛]8;;␛\␛[0m␊

and then if I copy that last line of text, and replace ␛ with \x1b I can create a printf statement that works (in iTerm2).

printf "\x1b[38;5;4m\x1b]8;;file:///Users/dan/src/delta/src/tests/test_example_diffs.rs\x1b\src/tests/test_example_diffs.rs\x1b]8;;\x1b\\x1b[0m\n"

Are you able to do an experiment along those lines?

[daniel-liuzzi]

Hey @dandavison,

Thanks for the quick response! I'm sorry I forgot to mention earlier that this is PowerShell 7.0.3, so I don't have bat:

❯ git show dd88342018aa239c77d7667bc988f0ff5db4610a | delta | bat --plain -A
bat: The term 'bat' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Is there a PowerShell equivalent or you meant for me to try this in something like WSL 2?

[dandavison]

Right, I'm not familiar with PowerShell, but anything that will "display non-printing characters" from standard input. In a POSIX environment the standard option would be cat -A. But really anything that can print out its standard input in a way that shows you exactly what bytes are present. Perhaps sending the output of delta to file and then examining it? I.e. the equivalent of

git show dd88342018aa239c77d7667bc988f0ff5db4610a | delta > delta-output.txt

[daniel-liuzzi]

Funny how that's exactly what I was trying 😄

git --no-pager show dd88342018aa239c77d7667bc988f0ff5db4610a | delta > test.txt

When I run cat test.txt it outputs a bunch of garbage, but links do work, strangely enough:

But then when I ctrl+click them, this happens:

Bummer…

---

In any case, here's test.txt.

[daniel-liuzzi]

microsoft/terminal#7699

🤞

[torarnv]

I'm seeing the same problem here on macOS. The issue seems to be in the pager. If I do git log -p | delta the escape character is stripped. If I do git log -p | delta | cat the links work (but no pager).

Does delta use a built in pager or does it fall back to less? (which strips the links: gwsw/less#75)

[torarnv]

Can be worked around by passing -r to less to pass through all escape codes, instead of just ANSI colors with -R

[torarnv]

E.g. via BAT_PAGER="less -rFX"

[dandavison]

Hi @torarnv

Can be worked around by passing -r to less to pass through all escape codes, instead of just ANSI colors with -R

Hey, that's neat! I don't think I knew about that. Are there any drawbacks to that? I'm confused, maybe I've forgotten the issues involved. If -r works then why is gwsw/less#54 needed?

I'm seeing the same problem here on macOS. The issue seems to be in the pager
Does delta use a built in pager or does it fall back to less?

Yes, delta uses less as its pager. This is what delta's help text currently says (maybe it should be changed to mentioned -r?)

Hyperlinks are supported by several common terminal emulators.
However, they are not yet supported by less, so they will not work in
delta unless you install a patched fork of less (see
https://github.com/dandavison/less).

[torarnv]

Can be worked around by passing -r to less to pass through all escape codes, instead of just ANSI colors with -R

Hey, that's neat! I don't think I knew about that. Are there any drawbacks to that? I'm confused, maybe I've forgotten the issues involved. If -r works then why is gwsw/less#54 needed?

Reading the less manual, it says:

Warning: when the  -r  option  is used, less cannot keep track of the actual appearance of the screen (since this 
depends on how the screen responds to each type of control character).  Thus, various display prob- 
lems may result, such as long lines being split in the wrong place.

So I'm guessing that the idea behind -R is to allow only the color escape codes though, which would avoid any escape codes messing up the terminal size and similar, while still allowing colored output.

So your patch to less's -R still makes sense I think, as it would be in the same group as the ANSI color codes. But I don't know if that would be acceptable upstream since it's clearly documented to be "only ANSI "color" escape sequences are output in "raw" form".

I notice the manual also talks about theLESSANSIENDCHARS and LESSANSIMIDCHARS env variables which can modify the behavor of -R. Perhaps an easier patch to upsteam would be one adding a new env var that allows specifying the accepted control codes for -R? That would be backwards compatible for less, and forwards compatible if this problem appears for some other nice control char we don't want less to filter out.

I'm seeing the same problem here on macOS. The issue seems to be in the pager
Does delta use a built in pager or does it fall back to less?

Yes, delta uses less as its pager. This is what delta's help text currently says (maybe it should be changed to mentioned -r?)

But it uses less via batright? Because setting BAT_PAGER="less -rFX" helped. I think that would be a good distinction to make in the readme, i.e. how delta relies on bat.

Hyperlinks are supported by several common terminal emulators.
However, they are not yet supported by less, so they will not work in
delta unless you install a patched fork of less (see
https://github.com/dandavison/less).

Ah, I missed that one! Including -r here would be nice yes, as most people won't go build a patched less 😊

[dandavison]

But it uses less via bat right? Because setting BAT_PAGER="less -rFX" helped. I think that would be a good distinction to make in the readme, i.e. how delta relies on bat.

Actually, delta does not use bat in any way. (But delta has borrowed some code from bat for spawning the pager process.) However, as you say, delta does honor the BAT_PAGER environment variable. I did that because I thought it would be convenient for people, but I know it might also be a bit confusing/misleading. We could introduce a DELTA_PAGER env var and give it precedence over BAT_PAGER, but so far no-one has asked for that.

[dandavison]

So I'm guessing that the idea behind -R is to allow only the color escape codes though, which would avoid any escape codes messing up the terminal size and similar, while still allowing colored output. ... So your patch to less's -R still makes sense I think, as it would be in the same group as the ANSI color codes.

Ah of course, that's right. Thanks for explaining that.

I notice the manual also talks about the LESSANSIENDCHARS and LESSANSIMIDCHARS env variables which can modify the behavor of -R. Perhaps an easier patch to upsteam would be one adding a new env var that allows specifying the accepted control codes for -R? That would be backwards compatible for less, and forwards compatible if this problem appears for some other nice control char we don't want less to filter out.

OK, that's an interesting idea (tangentially: delta actually makes use of LESSANSIENDCHARS). I suppose the argument for that approach depends on how often false-positives (undesirable behavior) would result if -R suddenly gains the ability to pass on the OSC hyperlink codes? I'm not sure what the less development process is like and which of these changes if any are likely to be released. But could be worth mentioning your idea to less developers?

[torarnv]

But it uses less via bat right? Because setting BAT_PAGER="less -rFX" helped. I think that would be a good distinction to make in the readme, i.e. how delta relies on bat.

Actually, delta does not use bat in any way. (But delta has borrowed some code from bat for spawning the pager process.) However, as you say, delta does honor the BAT_PAGER environment variable. I did that because I thought it would be convenient for people, but I know it might also be a bit confusing/misleading.

Ah, interesting! 😊 A bit confusing yes 😊

We could introduce a DELTA_PAGER env var and give it precedence over BAT_PAGER, but so far no-one has asked for that.

That sounds like a good idea! I don't know if you already pick up PAGER too, but checking DELTA_PAGER, BAT_PAGER, and PAGER in order before falling back to less would be great.

[torarnv]

OK, that's an interesting idea (tangentially: delta actually makes use of LESSANSIENDCHARS). I suppose the argument for that approach depends on how often false-positives (undesirable behavior) would result if -R suddenly gains the ability to pass on the OSC hyperlink codes? I'm not sure what the less development process is like and which of these changes if any are likely to be released. But could be worth mentioning your idea to less developers?

Good idea, added a comment in the upstream bug.

[dandavison]

That sounds like a good idea! I don't know if you already pick up PAGER too, but checking DELTA_PAGER, BAT_PAGER, and PAGER in order before falling back to less would be great.

OK, done in 31e2661 (not released yet). (Yep, we were already checking PAGER.)