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

Use TERM_PROGRAM for determining WezTerm terminal #185

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

MuhammedZakir
Copy link
Contributor

@MuhammedZakir MuhammedZakir commented Jul 13, 2021

Currently, TERM is used, which is not set by default.

Ref: #182 (comment)

Edit: update manpage to mention WezTerm and fix typos.

@MuhammedZakir MuhammedZakir marked this pull request as draft July 13, 2021 11:40
@MuhammedZakir MuhammedZakir marked this pull request as ready for review July 13, 2021 12:21
src/terminal.rs Outdated Show resolved Hide resolved
@swsnr
Copy link
Owner

swsnr commented Jul 13, 2021

Thanks 🙏

Currently, `TERM` is used, which is not set by default.
Copy link
Owner

@swsnr swsnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, especially for the rigorous proof reading of the manpage 🙏 Much obliged 🤗

@swsnr swsnr merged commit ac81e6a into swsnr:main Jul 14, 2021
@MuhammedZakir MuhammedZakir deleted the wezterm branch July 15, 2021 04:06
@SuperSandro2000
Copy link
Contributor

It should fallback to TERM which it now no longer does.

The reason why wezterm does not set TERM by default is that it breaks WSL and ssh targets which do not have the terminfo installed and there is no easy way to automatically do it or forward that info.

@SuperSandro2000
Copy link
Contributor

Also $TERM_PROGRAM is some arbitrary environment variable which is not set by wezterm or is documented in any of my manpages.

@MuhammedZakir
Copy link
Contributor Author

Also $TERM_PROGRAM is some arbitrary environment variable which is not set by wezterm or is documented in any of my manpages.

Before changing, I did look at the code, and WezTerm do set TERM_PROGRAM and TERM_PROGRAM_VERSION[1][2]. Also, Wez himself said to use it [3].

[1] https://github.com/wez/wezterm/blob/ac5199c21663e39dcce887308a92b5e36b338866/config/src/lib.rs#L1711
[2] https://github.com/wez/wezterm/search?q=TERM_PROGRAM&type=commits
[3] wez/wezterm#230 (comment)

@SuperSandro2000
Copy link
Contributor

That only works if you are opening the shell locally on a Linux machine. Everyone needs to duct tape around this when using WSL or SSH. Just use TERM as a fallback instead of some random other environment variable.

@SuperSandro2000
Copy link
Contributor

Forwarding the variable over ssh is not that easy because it needs both changes in the client and server config of ssh. Also I am pretty sure there are pieces of software which will also needs special treatment which is totally unnecessary here.

@swsnr
Copy link
Owner

swsnr commented Jul 16, 2021

@SuperSandro2000 I am sorry but I don't like your tone; please try to be less combative and less passive aggressive. Thanks.

mdcat will continue to use TERM_PROGRAM because unlike TERM it works by default whereas TERM needs additional configuration, at least in my experience.

I understand that it doesn't work over SSH by default, but neither does TERM without copying terminfo files around. I'd also like to point out that iTerm detection also relies on TERM_PROGRAM only, and so far I haven't received any complaints about this.

I am not opposed to checking additional environment variables, but personally I have no need to change this, and please understand that you are not entitled to demand this from @MuhammedZakir.

If you need a fallback to TERM please do make a pull request yourself. I'd be happy to review it. Thanks.

@SuperSandro2000
Copy link
Contributor

If you need a fallback to TERM please do make a pull request yourself.

I am doing that right now.

@swsnr
Copy link
Owner

swsnr commented Jul 16, 2021

@SuperSandro2000 Thank you 🙏

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