Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

πŸ› Windows Terminal Preview 1.4: Hyperlinks not working #332

Closed
daniel-liuzzi opened this issue Sep 22, 2020 · 18 comments
Closed

πŸ› Windows Terminal Preview 1.4: Hyperlinks not working #332

daniel-liuzzi opened this issue Sep 22, 2020 · 18 comments

Comments

@daniel-liuzzi
Copy link

daniel-liuzzi commented Sep 22, 2020

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

image

Other information

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

image

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
Copy link
Owner

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

Are you able to do an experiment along those lines?

@daniel-liuzzi
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

daniel-liuzzi commented Sep 22, 2020

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:

image

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

image

Bummer…


In any case, here's test.txt.

@daniel-liuzzi
Copy link
Author

microsoft/terminal#7699

🀞

@torarnv
Copy link
Contributor

torarnv commented Oct 3, 2020

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
Copy link
Contributor

torarnv commented Oct 3, 2020

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

@torarnv
Copy link
Contributor

torarnv commented Oct 3, 2020

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

@dandavison
Copy link
Owner

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
Copy link
Contributor

torarnv commented Oct 3, 2020

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor

torarnv commented Oct 3, 2020

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
Copy link
Contributor

torarnv commented Oct 3, 2020

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
Copy link
Owner

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

@torarnv
Copy link
Contributor

torarnv commented Oct 4, 2020

Thank you!

@daniel-liuzzi
Copy link
Author

daniel-liuzzi commented Oct 14, 2020

I just tried delta 0.4.4 and can confirm the issue is now solved! πŸŽ‰

To recap, this is what I've done to have delta render hyperlinks in Windows Terminal & PowerShell Core:

  1. Enable hyperlinks in delta

    git config --global delta.hyperlinks true
    
  2. Add the following to PowerShell's $PROFILE (credit to @torarnv)

    $env:DELTA_PAGER = 'less -rFX'
    
  3. Enjoy

Note that clicking hyperlinks still doesn't work, but that's already tracked in microsoft/terminal#7699.

Thanks to @dandavison and @torarnv for helping out!

@Dkendal
Copy link

Dkendal commented Apr 18, 2022

Just commenting for posterity that I see the same thing on linux with kitty terminal version 0.25.0 and delta version 0.12.1. The less -r trick works for me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants