Add support for hyperlinks in kitty #165
Conversation
Kitty supports OSC8 (hyperlinks) in versions above 0.19.0.
|
@wladh Woa, nice 🤩 Been waiting for this, guess it's about time I switch my terminal emulator! I'll give this change a review soon'ish 🙂 |
There was a problem hiding this comment.
@wladh Thanks a lot for this PR. Aside from a few stylistic remarks concerning the capabilities constructor my major concern is how this PR checks for the Kitty version.
I think shelling out to kitty --version is not really reliable, and it's only half way through, because there's the allow_hyperlinks configuration setting to account for.
| .ok() | ||
| .filter(|value| value == "xterm-kitty")?; | ||
|
|
||
| let output = Command::new("kitty").arg("--version").output().ok()?; |
There was a problem hiding this comment.
This will fail if kitty is not in $PATH or doesn't exist at all (e.g. inside a chroot or systemd container). And worse, it won't fail gracefully in this case and detect kitty without hyperlinks; instead it'll make mdcat believe that it's not running in kitty at all, even if $TERM says it's kitty.
Can we somehow get the version in a more reliable way? Perhaps by means of some escape sequence?
And is the version actually sufficient? What about the allow_hyperlinks setting?
There was a problem hiding this comment.
If kitty is not in $PATH image display would also fail. You're right about allow_hyperlinks, but looking at the issue and at the commits in kitty I haven't seen anything implemented for feature detection.
There was a problem hiding this comment.
I wasn't aware that image display uses kitty. In this case I agree that it's a good idea to fail completely if kitty isn't found! But please document this in the docstring of the method, so that I don't forget and think it's a bug in six months 😉
| @@ -128,11 +128,15 @@ impl TerminalCapabilities { | |||
| } | |||
|
|
|||
| /// Terminal capabilities of Kitty. | |||
| pub fn kitty() -> TerminalCapabilities { | |||
| pub fn kitty(version: (u8, u8, u8)) -> TerminalCapabilities { | |||
There was a problem hiding this comment.
I'd like for these constructors to always return the same set of capabilities.
Would you mind to leave pub fn kitty() as is, and add a separate and independent pub fn kitty_with_links()? And the move the condition either into a separate pub fn kitty_autodetect(version) or even just to detect()?
|
Why do you need to detect at all? Simply always output hyperlinks if on kitty, earlier versions of kitty will ignore them and display text, newer versions will hyperlink the text. Unless you are changing the output of mdcat based on detecting hyperlink support. In which case I am happy to discuss modalities for detection, although they would be kitty specific, since the spec author refused to add any detection facilities to the spec. |
In fact we do @kovidgoyal. If mdcat "detects" hyperlink support it only prints the link text. If the terminal swallows the URL the user will have no chance to even see it. We do have an (albeit private) flag to emit only plain ANSI format which would let the user work around this, but I would prefer if mdcat would only use hyperlinks if it's reasonably sure the underlying terminal supports it. But "detection" is more "guesswork" as far as |
|
I've already added escape code based detection, see the hyperlink ticket |
@wladh Can you check whether that's feasible for mdcat?
@kovidgoyal I don't see things so rigorous 🤷 Environment variables are also simple to use, and fail safely. At worst you'll get Ansi formatting out of mdcat, but it won't crash or mess up the terminal with some weird escape stuff. And what good's a nice escape sequence if we can't use it because Sure environment variables aren't perfect, but from my point of view this is a "a good guess is good enough" business 😎 |
|
On Wed, Oct 07, 2020 at 07:11:08AM -0700, Basti wrote:
@kovidgoyal I don't see things so rigorous 🤷 Environment variables are also simple to use, and fail safely. At worst you'll get Ansi formatting out of mdcat, but it won't crash or mess up the terminal with some weird escape stuff.
Neither will escape codes, unless mdcat crashes, in which case the
terminal is hosed anyway. There is nothing *weird* about this escape
code.
And what good's a nice escape sequence if we can't use it because `$TERM` didn't make it across whatever connection we have? What good's an escape sequence if printing images requires `kitty icat` which doesn't make it across SSH either? 🙂
TERM is propagated across SSH. And it has no bearing on using the escape
code. You can use the escape code regardless of TERM.
Sure environment variables aren't perfect, but from my point of view this is a "a good guess is good enough" business 😎
If you dont want to use escape codes, just output the hyperlinks if
kitty is present in TERM.
|
|
On Wed, Oct 07, 2020 at 07:11:08AM -0700, Basti wrote:
And what good's a nice escape sequence if we can't use it because `$TERM` didn't make it across whatever connection we have? What good's an escape sequence if printing images requires `kitty icat` which doesn't make it across SSH either? :slightly_smiling_face:
And you dont need kitty icat to display images. You can output the
escape codes for doing so yurself.
|
That was proverbial, there's no need to lecture me 🙂
We need to be reasonably sure the underlying terminal understands the escape sequence, in particular if we expect to receive a response. |
But do, but we still need |
|
On Wed, Oct 07, 2020 at 08:54:07AM -0700, Basti wrote:
> You can use the escape code regardless of TERM.
We need to be reasonably sure the underlying terminal understands the escape sequence, in particular if we expect to receive a response.
No, you dont. You cant assume you will get a response in any case since
this can potentially be over a arbitrarily slow network link. So you
send the escape code, and wait a little while for a response, if none is
received, you proceed using whatever heuristics you have available to
you.
Although for a cat like utility, if I were you, I would just output the
hyperlinks by default and leave it up to the user to configure whether
they want them or not. Delay in catting is not really desirable. Anyway,
its your utility, do what you feel like. As far as kitty is concerned,
there is not going to be an env var just for hyperlink detection.
Detection via escape code is implemented, and if desired, I can add a
custom field for it to the terminfo database as well, but that is about
as far as I am willing to go.
|
|
On Wed, Oct 07, 2020 at 08:56:19AM -0700, Basti wrote:
> And you dont need kitty icat to display images. You can output the escape codes for doing so yurself.
But do, but we still need `kitty icat --print-window-size` to get the window size. Perhaps there's also an escape sequence for that, but it's how it is currently, and I don't have the time to change it.
No, you dont. Getting the window size is a simple piece of synchronous
five line code, demoed here: https://sw.kovidgoyal.net/kitty/graphics-protocol.html#getting-the-window-size
|
Obviously 👍 but that's not my point 🙂 I just wouldn't print the escape sequence in the first place if I'm not reasonably sure that the terminal understands it and returns a good response in return if it gets the chance. I wouldn't trust a terminal to handle anything it doesn't understand well. I do concede though that XTGETTCAP is a fairly standard sequence that all terminals should at least ignore safely. But I'd still check $TERM first 🙂
I guess we can afford some 100ms to check for hyperlinks, for I presume no one's going to use mdcat through SSH over a slow link; even if it might survive feature detection it's gonna stall anway as soon as it tries to transmit an entire uncompressed image embedded in escape sequences 🙈 I care more about having a good default, and to me this implies we automagically enable hyperlinks if the underlying terminal supports. I don't want to think about this myself, or mess with my shell aliases 🤷 This tool very much follows my own personal preferences, because my own use is still what this is mostly written for 😊
That's fine for me, and I didn't intend ask for any different. I'm sorry if you understood me different. I just suggested an env var in the first place because I thought it'd be the simplest way, and I wasn't aware that kitty had other means to detect hyperlink support. I'm fine with environment variables but I'm also fine with any other means of making a good guess whether Kitty supports links. I applaud you for doing things the right way(TM) in Kitty, but as far as mdcat's concerned we're using env vars all over the place anyway since no other terminal goes this far to make things "right". 🙈 If everyone would properly respond to XTGETTCAP I'd perhaps be a happier person, but then again none yet complained that we did the wrong thing in iTerm or Gnome Terminal 😉, and I have to admit that I have many more pressing things to worry about 🙂 But there's still no need to lecture me, so I'd appreciate if did a little less "No, you don't" and spoke a little less condescending, and just a tad more friendly.
The person who contributed image support for Kitty did. Perhaps they weren't aware of this ioctl, perhaps they considered shelling out easier and safer than ioctls, I don't know. But things are as they are and as I said I don't have time to change it. Will you open a pull request? I don't think so, and I don't expect you to, but I would like to ask you to speak less condescending about work people contribute in their own free time for the benefit of others. I mean no offence but they do deserve better 🙂 |
|
On Wed, Oct 07, 2020 at 09:47:46AM -0700, Basti wrote:
I care more about having a good default, and to me this implies we automagically enable hyperlinks if the underlying terminal supports. I don't want to think about this myself, or mess with my shell aliases :shrug:
I'm suggesting you always output hyperlinks, the vast majority of
terminals will ignore them safely, and leave it up to users to configure
if they dont want hyperlinks. But again, your tool, do what you like.
But there's still no need to lecture me, so I'd appreciate if did a little less "No, you don't" and spoke a little less condescending, and just a tad more friendly.
"No, you dont", isn't condescending. Sorry you feel that way. Anyway, I am
bowing out now. Good luck.
|
As said, mdcat doesn't print the URL if it uses hyperlinks, so this would leave people on terminals without hyperlink support without any way to actually see the URL. Sure, they can configure it, but it still makes a poor default in my opinion, in particular since some popular terminals do not yet support hyperlinks (e.g. xterm.js which is what VSCode uses as far as I know, or KDE Konsole). I do very much hope that this makes a safe default at some point but I don't think we're there yet, even if you just consider "modern" terminal emulators.
I guess we'll have to agree to disagree here, but thank you for your apology 🙏
To you as well. Thank you for the work you do on Kitty 👍 It is truly an awesome tool, and easily one of the best terminal emulators out there 👏 |
|
@wladh I've followed @kovidgoyal suggestions, and added it without any kind of feature detection. If anyone needs feature detection they can open another pull request 🙂 @kovidgoyal While I'm at this I'm sorry to bother you with another question: Kitty doesn't happen to support an escape sequence to set a mark on a certain line number, like iTerm does? In Iterm2 mdcat places such marks on every Markdown header to allow jumping back and forth between sections. |
|
On Fri, Oct 16, 2020 at 12:32:45AM -0700, Basti wrote:
@kovidgoyal While I'm at this I'm sorry to bother you with another question: Kitty doesn't happen to support an escape sequence to set a mark on a certain line number, like iTerm does? In Iterm2 mdcat places such marks on every Markdown header to allow jumping back and forth between sections.
No there is no such facility.
|
|
@kovidgoyal Sorry to hear this. Thanks for your fast answer 🙏 |
|
Sorry, I didn't have time to get back to this. Thanks for implementing it. |
Kitty supports OSC8 (hyperlinks) in versions 0.19.0 and later.