Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Add support for WezTerm #182

Merged
merged 1 commit into from Jul 2, 2021
Merged

Add support for WezTerm #182

merged 1 commit into from Jul 2, 2021

Conversation

MuhammedZakir
Copy link
Contributor

@MuhammedZakir MuhammedZakir commented Jun 28, 2021

Wez's Terminal Emulator - https://wezfurlong.org/wezterm/

Uses iTerm image protocol for rendering images


IMO, terminal specific files in src/terminal/ dir can be modified to make it as "protocol/features" specific code. And then, we could use is_term_env_var function, with slight modification, for detecting other terminals (except VTE). What do you think?


Btw, thanks for this cool app! :-)

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.

In addition to my remake I'd appreciate if you could add this to CHANGELOG.md.

I appreciate your suggestions but I am not convinced that this refactoring would have any benefit. In particular the TERM variable does not identify the terminal emulator per se; instead it simply denotes the terminal capabilities the terminal emulator would like to announce.

While some emulators do announce custom values and inject the corresponding entries into the term cap database many others simply announce a known value, mostly xterm 256, to maintain compatibility with existing programs.

Since the term cap database doesn't support any of the more recent extensions to terminal protocols (true colours, inline images, links, etc) I find the value of extending TERM with custom values questionable, and would rather rely on alternative means to identify the actual terminal emulator.

I understand that this position is debatable but unless the features supported by mdcat promote from proprietary extensions of terminal emulators to general standards with termcap entries which we can detect in a generic way through terminfo I see little value in making TERM checks a generic thing in mdcat.

src/terminal.rs Outdated Show resolved Hide resolved
src/terminal.rs Outdated Show resolved Hide resolved
@swsnr
Copy link
Owner

swsnr commented Jun 28, 2021

Please do also check the Github actions error, and do fix it.

Thanks

@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Jun 28, 2021

Thanks.

Welcome! :-)

In addition to my remake I'd appreciate if you could add this to CHANGELOG.md.

Please do also check the Github actions error, and do fix it.

Will do.

[... about TERM var ...]

Sorry! I didn't make it clear when I said "with slight modification". What I meant was to change that funcion to

func (env_var, expected_value) -> bool

then we could use it as

func ("TERM", "xterm-kitty")
func ("TERM", "wezterm")
func ("TERM-PROGRAM", "iTerm.app")
...

Of course, this is not necessary, as there are only few terminals atm which support the features mdcat wants.

@swsnr
Copy link
Owner

swsnr commented Jun 28, 2021

@MuhammedZakir Wrt to the actions failure I have removed a few needless borrows on main; please rebase onto main and the error should be gone 🙂

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.

Please also check why the rustfmt pipeline step complains.

CHANGELOG.md Outdated Show resolved Hide resolved
src/terminal.rs Outdated Show resolved Hide resolved
src/terminal.rs Outdated Show resolved Hide resolved
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.

I forgot to ask but could you add Wezterm to feature table in README as well?

@MuhammedZakir
Copy link
Contributor Author

I forgot to ask but could you add Wezterm to feature table in README as well?

Done.

Btw, I wasn't trying to be lazy regarding doc comments. I haven't written much code, so am not sure what should be doc comments, and what should be code comments. So feel free to say if you need to change anything. Also, regarding punctuation, for single line comments, I have seen with and without ending dot. I have corrected it now. Is this a Rust convention? Anyway, I just wanna say, this is due to my inexperience, and not laziness. I hope I didn't create much trouble for you! :-)

@swsnr
Copy link
Owner

swsnr commented Jul 2, 2021

@MuhammedZakir Oh, I didn't mean to imply you were lazy. I'm sorry for this misunderstanding.

You couldn't have known; these are just my personal preferences after all.

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.

A minor last thing about the CHANGELOG.

And please do squash all commits into a single one (e.g. with git rebase -i).

CHANGELOG.md Outdated Show resolved Hide resolved
Wez's Terminal Emulator - https://wezfurlong.org/wezterm/

Uses iTerm image protocol for rendering images
@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Jul 2, 2021

I just found out that WezTerm supports "mark" using semantic prompts [1]. Unlike iTerm2, this one needs ending marker.

Prompt mark start: 133;P;k=s
Prompt mark end: 133;B

What is the right way for implementing this?

[1] https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md

@swsnr
Copy link
Owner

swsnr commented Jul 2, 2021

That's a different kind of mark which is of no use to mdcat 🙂

@swsnr swsnr merged commit 6fa7bfa into swsnr:main Jul 2, 2021
@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Jul 2, 2021

That's a different kind of mark which is of no use to mdcat slightly_smiling_face

AFAIK, technically, they are.

https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md:

This specification is an extension of the protocol first implemented by FinalTerm and then later by iterm2. Other similar prior art is known for Extraterm, DomTerm, XMLterm, and GraphTerm.

https://iterm2.com/documentation-shell-integration.html:

How it works

Shell Integration works by configuring your shell on each host you log into to send special escape codes that convey the following information:

  • Where the command prompt begins and ends.
  • Where a command entered at the command prompt ends and its output begins.
  • The return code of the last-run command.
    [...]

This is the exact same thing mentioned in semantic-prompts proposal, except, iTerm2 sometimes mention "history location", and the proposal always mention "REPL". I understand what both of them "stressed" is different, but from what I can understand, adding markes using semantic-prompt is much easier as there is support for application identifier and prompt kinds [1].

IMHO, it's worth adding. After all, it's very useful. :-)

Edit: [1] what I meant here is, we could have different keyboard shortcuts - one for shell prompts, and 1+ for everything else, making it much easier to navigate.

@swsnr
Copy link
Owner

swsnr commented Jul 2, 2021

@MuhammedZakir I am sorry but they are not; mdcat uses the proprietary iTerm escape code OSC 1337 with a SetMark command to place arbitrary marks (see Proprietary Escape Codes).

You're referring to the FTCS_PROMPT and FTCS_COMMAND_* escape codes which specifically mark the beginning and the end of the prompt, and only that. Modern terminal emulators rely on these escape codes to identify where the prompt is and whether a command is running in the terminal window; mdcat must not emit these codes lest it confuses the terminal emulator.

@MuhammedZakir
Copy link
Contributor Author

Hmm... :-(

@MuhammedZakir MuhammedZakir deleted the wezterm branch July 2, 2021 18:20
@swsnr
Copy link
Owner

swsnr commented Jul 11, 2021

@MuhammedZakir Does this change actually work for you? I tried a recent wezterm release, and it doesn't set TERM as this PR assumes.

@MuhammedZakir
Copy link
Contributor Author

@MuhammedZakir Does this change actually work for you? I tried a recent wezterm release, and it doesn't set TERM as this PR assumes.

You'll have to set it manually.

  1. Support for undercurl wez/wezterm#415
  2. https://wezfurlong.org/wezterm/config/lua/config/term.html
  3. (same as #1) https://wezfurlong.org/wezterm/faq.html#how-do-i-enable-undercurl-curly-underlines

@swsnr
Copy link
Owner

swsnr commented Jul 13, 2021

@MuhammedZakir Okay, but why didn't you use TERM_PROGRAM which wezterm sets automatically?

@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Jul 13, 2021

@MuhammedZakir Okay, but why didn't you use TERM_PROGRAM which wezterm sets automatically?

I didn't know that. I thought it was only set it in macOS. Does (cross-platform?) terminals normally set that in Linux?

P.S. I can send a PR to fix this. Do we need to check if either (TERM & TERM_PROGRAM) have wezterm, or just use TERM_PROGRAM?

@swsnr
Copy link
Owner

swsnr commented Jul 13, 2021

Just TERM_PROGRAM, just like for iTerm.

I don't think it's a standard variable, but as far as I can see wezterm sets it always 🙂

Would be really cool if you could send a PR for this 🙏😇

@swsnr
Copy link
Owner

swsnr commented Jul 13, 2021

See e.g.

$ env | rg -i wezterm
GIO_LAUNCHED_DESKTOP_FILE=/usr/share/applications/org.wezfurlong.wezterm.desktop
TERM_PROGRAM=WezTerm
WEZTERM_UNIX_SOCKET=REDACTED
WEZTERM_EXECUTABLE_DIR=/usr/bin
WEZTERM_PANE=0
WEZTERM_CONFIG_FILE=REDACTED
WEZTERM_EXECUTABLE=/usr/bin/wezterm-gui
WEZTERM_CONFIG_DIR=REDACTED

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants