Add support for hyperlinks

[ltrzesniewski]

I'd like to implement hyperlink support in ripgrep (BurntSushi/ripgrep#665), and this is the first step, as per your comment @BurntSushi.

I had to make some design choices here, which I can adjust after getting your input:

• Currently, reset does not reset the hyperlink, only the color. That's not necessarily the best choice, and I'm not sure what to do here, so I need your guidance. My reasoning was:

  • Colors and hyperlinks are cleanly split.
  • The "auto-reset" feature (set_reset) would get in the way, and calling set_color could reset the hyperlink, which would be unexpected. I could only reset the color in that case though. On the other hand, reset not resetting everything is also unexpected. 😕
  • The reset code would be much longer ("\x1B[0m\x1B]8;;\x1B\\" instead of "\x1B[0m"), not sure if that's an issue. Maybe that could be mitigated by adding some state in Ansi, so it would only emit the hyperlink reset code if previously emitted a hyperlink?

• An alternate implementation would be a write_hyperlink function which takes the URI and text to write in a single call, but that would prevent from using multiple colors in a hyperlink easily. I suppose another function which takes a closure would need to be provided in order to handle this case.

• I didn't add support for link parameters yet. These can be added later to the HyperlinkSpec type:

  • Either as a generic key/value map
  • Or only with support for known parameters (currently, only id is defined)

So, please tell me which solution you prefer and I'll implement it. 🙂

I tested this with Windows Terminal.

[ltrzesniewski]

I added a simple write_hyperlink extension which should be sufficient for most usages.

I tried to add something like this, in order to support multi-colored links and get rid of set_hyperlink:

writer.write_hyperlink(|w| {
	w.set_color(...)?;
	w.write_all(directory)?;
	w.set_color(...)?;
	w.write_all(file)
});

But due to wrappers such as LossyStandardStream<WriterInner<IoStandardStream>>, I couldn't find a way to make this work, probably due to my lack of experience with the language. See here for more details.

[BurntSushi]

• I didn't add support for link parameters yet. These can be added later to the HyperlinkSpec type:

Is any of this necessary to get hyperlink support working in ripgrep? Basically, would it make sense to land this without doing this work?

(I'm finally digging into this hyperlink issue and trying to make sense of everything. :-))

[ltrzesniewski]

I don't think hyperlink parameters are necessary at all TBH. The hyperlink spec defines them, but AFAIK they aren't used anywhere. It only states: "These parameters allow future extendability of this feature."

BTW, I ended up not using write_hyperlink in ripgrep. It may be useful for other uses, but you may prefer to remove it.

Oh, I also merged master into my branch, sorry. 😅 I squashed everything into a single commit.

[BurntSushi]

Yeah squashing everything down into one commit would be great. And definitely remove anything that isn't necessary. :-)

[ltrzesniewski]

Done 🙂

[BurntSushi]

Thank you!

[BurntSushi]

So looking at this more closely, it seems like the set_hyperlink method should just be named write_hyperlink. It isn't like set_color because it doesn't really mutate some state. It introduces the hyperlink escape code, writes the hyperlink given and then closes the hyperlink.

The other thing I'm confused about is why the hyperlink is just a link and doesn't have a label. How does the label get set? I'd expect that to be an ANSI escape code too? I haven't quite done the full background reading yet...

[BurntSushi]

Oh... I see. Wow. The example in this doc helped explain things. So basically, after set_hyperlink is called with a URI, the caller is then free to write the label text. And the caller then "closes" the hyperlink by calling set_hyperlink with a HyperlinkSpec::none().

So write_hyperlink is indeed the wrong name and this is mutating state in a fashion similar to set_color.

[BurntSushi]

The other thing that is rather annoying here is that this apparently does not work at all in tmux. I saw your issue here, and it's closed. I built revision b202a2f1 from git for tmux, but still no dice. So I don't know what's going on there. And I can't even follow up there and ask about it because they locked the issue. Sigh.

[BurntSushi]

OK, figured out the tmux issue: https://github.com/BurntSushi/dotfiles/blob/4f29beb508708e8a88676ffa194f0dc9c19057ae/.tmux.conf#L17-L20

Thanks to this: tmux/tmux#3660

[BurntSushi]

I've changed the docs on set_hyperlink to this:

    /// Set the current hyperlink of the writer.
    ///
    /// The typical way to use this is to first call it with a
    /// [`HyperlinkSpec::open`] to write the actual URI to a tty that supports
    /// [OSC-8]. At this point, the caller can now write the label for the
    /// hyperlink. This may include coloring or other styles. Once the caller
    /// has finished writing the label, one should call this method again with
    /// [`HyperlinkSpec::close`].
    ///
    /// If there was a problem setting the hyperlink, then an error is returned.
    ///
    /// [OSC8]: https://github.com/Alhadis/OSC8-Adoption/

I was very confused about how to use this API until I actually started reading about OSC8 and actually trying an example. I tried to re-word the docs to target my confusion as I suspect others will be confused.

Notice also that I rename HyperlinkSpec::new to HyperlinkSpec::open and HyperlinkSpec::none to HyperlinkSpec::close. I think this more closely matches how one might want to use this and also makes more sense I think if you have a background with markup languages such as HTML.

[BurntSushi]

I also removed the Default impl for HyperlinkSpec because it seems a little weird to have it default to closing a link? I guess maybe if you think about "close" as "reset" instead, then it might make more sense. But I'm unsure and I don't think the default impl really carries its weight at the moment. We can always add it back later.

[BurntSushi]

And the new trait method supports_hyperlinks is a breaking change. Callers might have impls of WriteColor and introducing new required methods on an unsealed trait is a definitive semver major change. I've changed it to a default method that always returns false instead.

[BurntSushi]

And of course, adding set_hyperlink as a new required method is also a semver breaking change. So I changed that to a default method that always returns Ok(()). i.e., a no-op.

[ltrzesniewski]

it seems like the set_hyperlink method should just be named write_hyperlink

You figured it out, but FYI write_hyperlink was a helper func that I removed yesterday since it wasn't used in ripgrep and you asked me to remove unnecessary stuff, see this diff.

Apparently removing it made the API more confusing. 😕

The other thing that is rather annoying here is that this apparently does not work at all in tmux.

I had to run tmux with tmux -T hyperlinks to display hyperlinks, but I thought that was because of WSL. The command line is in the tmux issue, but it doesn't really stand out.

Notice also that I rename HyperlinkSpec::new to HyperlinkSpec::open and HyperlinkSpec::none to HyperlinkSpec::close.

I agree it's better that way. 👍

I also removed the Default impl for HyperlinkSpec because it seems a little weird to have it default to closing a link?

I wasn't sure about that one, I thought it was idiomatic to add Default where it more or less makes sense.

After you renamed HyperlinkSpec::none to HyperlinkSpec::close I'd say it makes less sense to have a Default indeed.

And the new trait method supports_hyperlinks is a breaking change.

I should have thought about introducing a default implementation. 👍

Thanks for handling this! 🙂