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

fix: add intentional switch fallthrough for tmux #14

Closed
wants to merge 2 commits into from

Conversation

Mause
Copy link

@Mause Mause commented Jan 21, 2025

See tmux/tmux#3468 for context

@Mause
Copy link
Author

Mause commented Jan 22, 2025

Ah, looks like this is intentional, per a27db48

@savioxavier
Copy link
Owner

Thank you for your pull request, but I'm not entirely sure what you want me to do here

On one hand, as you have mentioned, tmux overrides the TERM_PROGRAM variable. This is, well, rather unfortunate, as it would disallow users running tmux not viewing hyperlinks, even on supported terminals

On the other hand, I feel like doing this might not be the best idea. Say, someone is using tmux but on an unsupported terminal, and since we are only checking (in this PR) whether TERM_PROGRAM == "tmux", it wouldn't go down well if the user sees random escape codes strewn across the terminal screen

Moreover, the default: return false was added as a precaution. We wanted to default to true if and only if the terminal had full support for hyperlinks. This, once again, prevents the user from seeing random escape codes in the terminal

There does not seem to be a lot of options to rectify this issue, unless we could somehow get the underlying terminal name from some other variable. Apparently, the person who created that issue on the tmux repo seems to have found out a workaround by executing show-environment TERM_PROGRAM inside tmux. However, since termlink is supposed to be as lightweight as possible, I believe it wouldn't be a good idea to actually execute and capture outputs of commands, rather just check for the presence of certain environment variables. Not to mentioned the added complexity of figuring out a failsafe way of capturing command execution output from tmux.

I will wait until you have any comments to add, or alternative implementations to contribute, otherwise I think I will close this PR

@Mause
Copy link
Author

Mause commented Jan 28, 2025

Hey there,

the argument that the tmux maintainer makes in the linked issue is that tmux should itself be considered a terminal, which I don't really agree with, but that's neither here nor there.

the change i've made here, is if the TERM_PROGRAM is set to tmux, to ignore that information, and use the other information that is available to determine the "real" terminal that the user is using. unless i misunderstand how switch statements work in go, that's that the change should enable. there don't seem to be any tests for in place for me to extend to demonstrate this however

happy to take feedback to modify the changes :)

@thomas-maurice
Copy link

I'd like to add another (empirical) datapoint, I am running tmux on ghostty (osX/arm64) and running an app with termlink (even the most basic one in the README, with the latest version of the lib) does not work even with FORCE_HYPERLINK=true (which after reading the code should force the link to be rendred irrespectively of terminal support), which would lead me to think that tmux might be stripping these links somehow

@savioxavier
Copy link
Owner

Hello

Apologies for the long wait, I have been rather busy as of late.

@Mause I was planning to merge your changes, but after reading @thomas-maurice's above comment on how tmux strips hyperlinks for some reason even after trying to enable the FORCE_HYPERLINK variable, I have realized that this hacky workaround simply wouldn't be enough. It's certainly concerning enough that tmux is counter productive when it comes to parsing environment variables and now, rendering links. I feel that if we need to support tmux, we need to have a better, stronger and more reliable code that handles it.

Even though I'm primarly a Windows user and I have never used tmux before, I can already see this is a challenge we would need to figure out in the future. I, too, disagree with the fact that the tmux developers consider tmux to be a terminal. Perhaps we can hope tmux improves the developer experience in the future.

Moreover, I would like to investigate why exactly tmux isn't forcing hyperlinks and why it is stripping them. This appears to be quite unsettling, as this goes against the very structure of any terminal, hyperlink supported or not.

As of now, I will be closing this pull request. If you have a better way to improve the termlink experience with tmux, please open an issue first so we can discuss this further. I'm open to suggestions. Thank you once again for your contribution.

@Mause
Copy link
Author

Mause commented Feb 11, 2025

I'd like to add another (empirical) datapoint, I am running tmux on ghostty (osX/arm64) and running an app with termlink (even the most basic one in the README, with the latest version of the lib) does not work even with FORCE_HYPERLINK=true (which after reading the code should force the link to be rendred irrespectively of terminal support), which would lead me to think that tmux might be stripping these links somehow

Which terminal emulator are you running tmux in though? I'm able to run the example start.go inside fish inside tmux inside iTerm2 and see the links successfully rendered and be clickable with set -x FORCE_HYPERLINK true (while holding Control on macOS anyways)

EDIT: additional context, tmux might actually strip these codes after all in some cases, in which case set -ga terminal-features "*:hyperlinks" is required

@Mause
Copy link
Author

Mause commented Feb 11, 2025

Just to demonstrate:

image

The dashes under the text indicate the links in iTerm2

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

Successfully merging this pull request may close these issues.

3 participants