Add Ghostty terminal hyperlink support #3
Conversation
piotrmurach
left a comment
piotrmurach
left a comment
There was a problem hiding this comment.
Hi Akshay,
Thank you for submitting this PR. 🙏
I'm all in favour of adding Ghostty support. I left a couple of comments, though. In particular, I'm curious about the version check. Would you have time to investigate?
lib/tty/link/terminals/ghostty.rb
Outdated
| # | ||
| # @api private | ||
| def version? | ||
| true |
There was a problem hiding this comment.
Is this accurate? I had a quick look through Ghostty issues, and it appears that version 1.0.0 adds hyperlink support.(see OSC8 Hyperlink Support). Other releases fixed some issues with hyperlinks, notably the 1.2.0. I'm wondering whether we should add a version limit that can be read from the TERM_PROGRAM_VERSION. However, the official first release was 1.0.0, so it may be impossible for people to install an older version. In which case, this would be fine. Any thoughts?
There was a problem hiding this comment.
Given support is present from Ghostty’s first official release, and later versions mostly fix issues rather than add the feature, returning true seems accurate for now.
There was a problem hiding this comment.
I haven't settled on this one yet. I need this decision embedded in the code somehow. One way is to change it to use a version higher than 1.0.0 explicitly. The advantage is that this would serve as a direct explanation to anyone reading, including myself in the future. The alternative is to keep the true value with a comment that says "available since official public release 1.0.0` or similar. What do you reckon?
There was a problem hiding this comment.
Lets codify it. Best documentation is the well written code 😉 .
spec/unit/terminals/ghostty_spec.rb
Outdated
| it "supports links without a version" do | ||
| env = env_with_name.merge({"TERM_PROGRAM_VERSION" => nil}) | ||
| ghostty = described_class.new(semantic_version, env) | ||
|
|
||
| expect(ghostty.link?).to eq(true) | ||
| end | ||
|
|
||
| it "supports links without the term program version env variable" do | ||
| ghostty = described_class.new(semantic_version, env_with_name) | ||
|
|
||
| expect(ghostty.link?).to eq(true) | ||
| end |
There was a problem hiding this comment.
Let's swap these to match the prior tests ordering.
There was a problem hiding this comment.
applied the suggested changes.
the-spectator
force-pushed
the
feat/ghostty
branch
from
33e1270 to
7778e6f
Compare
|
Thanks for the review @piotrmurach ! I have updated the code as per review comments. |
piotrmurach
left a comment
piotrmurach
left a comment
There was a problem hiding this comment.
Thank you for making the changes. We are nearly there.
lib/tty/link/terminals/ghostty.rb
Outdated
| # | ||
| # @api private | ||
| def version? | ||
| true |
There was a problem hiding this comment.
I haven't settled on this one yet. I need this decision embedded in the code somehow. One way is to change it to use a version higher than 1.0.0 explicitly. The advantage is that this would serve as a direct explanation to anyone reading, including myself in the future. The alternative is to keep the true value with a comment that says "available since official public release 1.0.0` or similar. What do you reckon?
the-spectator
force-pushed
the
feat/ghostty
branch
from
7778e6f to
5f9fbed
Compare
the-spectator
force-pushed
the
feat/ghostty
branch
from
5f9fbed to
f7ab9bb
Compare
2 participants
Closes #2
Describe the change
Add Ghostty terminal hyperlink support
Why are we doing this?
To extend the supported terminal list.
Requirements
masterbranch?